Csaba Ringhofer 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 4: (11 comments) Thanks for the 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: runtimeDeferredParamet > Should it be runtimeDeferredParameters_? Done 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. Done 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: originalArgs: i > The name doesn't match the actual parameter 'originalArgs'. Done 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 You are right, in Impala we use Object[] at all other places I found. I don't know what is more idiomatic in Java actually. http://gerrit.cloudera.org:8080/#/c/19499/3/fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java@237 PS3, Line 237: for (Object originalArg: originalArgs) { > The index 'i' is not used except for indexing originalArgs, so we could use Done 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. Done 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 th I think that printing both forms can be useful for special characters, so I kept it as it is. 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. Done 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. Done 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: select generic_throws_exception(); > We no longer return NULL, the comment should be updated. Done 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 a readded the test - I don't remember why I have removed it -- 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: 4 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: Fri, 03 Mar 2023 14:43:41 +0000 Gerrit-HasComments: Yes
