Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19507 )
Change subject: IMPALA-11854: ImpalaStringWritable's underlying array can't be changed in UDFs ...................................................................... Patch Set 8: (9 comments) http://gerrit.cloudera.org:8080/#/c/19507/8/fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java File fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java: http://gerrit.cloudera.org:8080/#/c/19507/8/fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java@26 PS8, Line 26: * values that map to StringValue in the BE. Can you mention the caching behavior? http://gerrit.cloudera.org:8080/#/c/19507/8/fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java@59 PS8, Line 59: ; optional: bufferCapacity_ could be set here with getLengthFromNativeHeap() http://gerrit.cloudera.org:8080/#/c/19507/8/fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java@84 PS8, Line 84: public int getCapacity() { : return bufferCapacity_; : } What happens if this is called before initialize()? http://gerrit.cloudera.org:8080/#/c/19507/8/fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java@94 PS8, Line 94: Arrays.copyOf(array_, newCap); Can't this be called while array_ is still EMPTY_ARRAY? http://gerrit.cloudera.org:8080/#/c/19507/8/fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java@104 PS8, Line 104: initalize(); optional: returning getLengthFromNativeHeap() would allow optimizing the case when the length is accessed, but the data is not http://gerrit.cloudera.org:8080/#/c/19507/8/fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java@116 PS8, Line 116: if (EMPTY_ARRAY == array_) { : initalize(); : } This could be skipped in this case, as the original array won't be used. http://gerrit.cloudera.org:8080/#/c/19507/8/testdata/workloads/functional-query/queries/QueryTest/generic-java-udf.test File testdata/workloads/functional-query/queries/QueryTest/generic-java-udf.test: http://gerrit.cloudera.org:8080/#/c/19507/8/testdata/workloads/functional-query/queries/QueryTest/generic-java-udf.test@358 PS8, Line 358: select increment( cast("a" as binary)); nit: extra space http://gerrit.cloudera.org:8080/#/c/19507/8/testdata/workloads/functional-query/queries/QueryTest/java-udf.test File testdata/workloads/functional-query/queries/QueryTest/java-udf.test: http://gerrit.cloudera.org:8080/#/c/19507/8/testdata/workloads/functional-query/queries/QueryTest/java-udf.test@371 PS8, Line 371: select increment("a"); Can you add a test with empty string? The UDF seems to handle this case and it would be nice to test if the executor works in this case. (same for the generic case) http://gerrit.cloudera.org:8080/#/c/19507/8/testdata/workloads/functional-query/queries/QueryTest/java-udf.test@378 PS8, Line 378: select increment( cast("a" as binary)); nit: extra space -- To view, visit http://gerrit.cloudera.org:8080/19507 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb28bd0dce7b0482c7abe1f61f245691fcbfe212 Gerrit-Change-Number: 19507 Gerrit-PatchSet: 8 Gerrit-Owner: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Comment-Date: Fri, 24 Feb 2023 16:57:59 +0000 Gerrit-HasComments: Yes
