Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19499 )
Change subject: IMPALA-11911: Fix NULL argument handling in Hive GenericUDFs ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/19499/3/fe/src/main/java/org/apache/impala/hive/executor/HiveUdfExecutorGeneric.java File fe/src/main/java/org/apache/impala/hive/executor/HiveUdfExecutorGeneric.java: http://gerrit.cloudera.org:8080/#/c/19499/3/fe/src/main/java/org/apache/impala/hive/executor/HiveUdfExecutorGeneric.java@75 PS3, Line 75: runtimeDeferredObjects Should it be runtimeDeferredParameters_? http://gerrit.cloudera.org:8080/#/c/19499/3/fe/src/main/java/org/apache/impala/hive/executor/HiveUdfExecutorGeneric.java@79 PS3, Line 79: private DeferredObject deferredNullParameter_; This could be static. http://gerrit.cloudera.org:8080/#/c/19499/3/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/19499/3/fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java@229 PS3, Line 229: inputBufferSize The name doesn't match the actual parameter 'originalArgs'. http://gerrit.cloudera.org:8080/#/c/19499/3/fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java@233 PS3, Line 233: Object args[] For the last two parameters, wouldn't the syntax Object[] arrName be more idiomatic for Java? http://gerrit.cloudera.org:8080/#/c/19499/3/fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java@237 PS3, Line 237: for (int i = 0; i < originalArgs.length; ++i) { The index 'i' is not used except for indexing originalArgs, so we could use a foreach loop. http://gerrit.cloudera.org:8080/#/c/19499/3/fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java@270 PS3, Line 270: Object See L233 about syntax. http://gerrit.cloudera.org:8080/#/c/19499/3/fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java@408 PS3, Line 408: errMsgs.add("Expected string: " + Bytes.toString(expectedBytes)); It's not new in this change but wouldn't it be nicer if we didn't report the result both as a string and a bytes object, only the one that is relevant? http://gerrit.cloudera.org:8080/#/c/19499/3/fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java@430 PS3, Line 430: Object originalArgs[], Object args[] See L233 about syntax. http://gerrit.cloudera.org:8080/#/c/19499/3/fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java@523 PS3, Line 523: Object See L233 about syntax. http://gerrit.cloudera.org:8080/#/c/19499/3/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/19499/3/testdata/workloads/functional-query/queries/QueryTest/generic-java-udf.test@127 PS3, Line 127: # IMPALA-1392: Hive UDFs that throw exceptions should return NULL We no longer return NULL, the comment should be updated. http://gerrit.cloudera.org:8080/#/c/19499/3/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/19499/3/testdata/workloads/functional-query/queries/QueryTest/java-udf.test@a107 PS3, Line 107: Was this test deleted intentionally, shouldn't we test that it now throws an exception, like in the case of "select throws_exception() from functional.alltypestiny"? -- To view, visit http://gerrit.cloudera.org:8080/19499 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I53238612f4037572abb6d2cc913dd74ee830a9c9 Gerrit-Change-Number: 19499 Gerrit-PatchSet: 3 Gerrit-Owner: Csaba Ringhofer <csringho...@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-Reviewer: Steve Carlin <scar...@cloudera.com> Gerrit-Comment-Date: Wed, 22 Feb 2023 12:54:55 +0000 Gerrit-HasComments: Yes
