Daniel Becker 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 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/19507/4/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/4/fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java@79 PS4, Line 79: length 'length' is superfluous, could use bufferCapacity_ directly. http://gerrit.cloudera.org:8080/#/c/19507/4/fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java@104 PS4, Line 104: equals If initialize() has already been called but the array is actually empty, this condition will be true, so we will call initialize() again. We could use EMPTY_ARRAY == array_ here to check for identity, not equality. http://gerrit.cloudera.org:8080/#/c/19507/4/fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java@117 PS4, Line 117: equals See L104. http://gerrit.cloudera.org:8080/#/c/19507/4/fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java File fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java: http://gerrit.cloudera.org:8080/#/c/19507/4/fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java@173 PS4, Line 173: // Allocate StringVal sizeof(StringVal) = 8 + 4 Optional: we could mention that the 8 bytes are the pointer and 4 bytes are for the length. http://gerrit.cloudera.org:8080/#/c/19507/4/java/test-hive-udfs/src/main/java/org/apache/impala/CachedWritablesUdf.java File java/test-hive-udfs/src/main/java/org/apache/impala/CachedWritablesUdf.java: http://gerrit.cloudera.org:8080/#/c/19507/4/java/test-hive-udfs/src/main/java/org/apache/impala/CachedWritablesUdf.java@25 PS4, Line 25: public class CachedWritablesUdf extends UDF { Could include a short description of what we test here and why, including the Jira number. Should also mention what we do in the 2-argument overload of evaluate(). http://gerrit.cloudera.org:8080/#/c/19507/4/java/test-hive-udfs/src/main/java/org/apache/impala/GenericCachedWritablesUdf.java File java/test-hive-udfs/src/main/java/org/apache/impala/GenericCachedWritablesUdf.java: http://gerrit.cloudera.org:8080/#/c/19507/4/java/test-hive-udfs/src/main/java/org/apache/impala/GenericCachedWritablesUdf.java@29 PS4, Line 29: public class GenericCachedWritablesUdf extends GenericUDF { Could include a short description of what we test here and why, including the Jira number. http://gerrit.cloudera.org:8080/#/c/19507/4/java/test-hive-udfs/src/main/java/org/apache/impala/GenericCachedWritablesUdf.java@30 PS4, Line 30: objectInspector Nit: should end with _. http://gerrit.cloudera.org:8080/#/c/19507/4/java/test-hive-udfs/src/main/java/org/apache/impala/GenericCachedWritablesUdf.java@37 PS4, Line 37: This function takes 1 argument. Could include in the error message how many arguments were provided. Also see L52. http://gerrit.cloudera.org:8080/#/c/19507/4/java/test-hive-udfs/src/main/java/org/apache/impala/GenericCachedWritablesUdf.java@56 PS4, Line 56: byte[] bytes; 'bytes' could be declared within the switch cases as it is not used outside. http://gerrit.cloudera.org:8080/#/c/19507/4/java/test-hive-udfs/src/main/java/org/apache/impala/GenericCachedWritablesUdf.java@73 PS4, Line 73: value Shouldn't it be "unexpected type"? http://gerrit.cloudera.org:8080/#/c/19507/4/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/4/testdata/workloads/functional-query/queries/QueryTest/generic-java-udf.test@350 PS4, Line 350: ---- QUERY Do you think it would make sense to add a test with a CHAR() type just for consistency, even though CHAR is not affected by this change? -- 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: 4 Gerrit-Owner: Peter Rozsa <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Tue, 21 Feb 2023 16:16:57 +0000 Gerrit-HasComments: Yes
