Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19304 )
Change subject: IMPALA-11549: Support Hive GenericUdfs that return primitive java types ...................................................................... Patch Set 4: (11 comments) Thanks, looks good. http://gerrit.cloudera.org:8080/#/c/19304/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19304/4//COMMIT_MSG@18 PS4, Line 18: probably inheriting in the opposite : direction would be more logical We could also have an abstract base class and a concrete class for both kinds of return types. http://gerrit.cloudera.org:8080/#/c/19304/4/fe/src/main/java/org/apache/impala/hive/executor/JavaUdfDataType.java File fe/src/main/java/org/apache/impala/hive/executor/JavaUdfDataType.java: http://gerrit.cloudera.org:8080/#/c/19304/4/fe/src/main/java/org/apache/impala/hive/executor/JavaUdfDataType.java@116 PS4, Line 116: // Only primitive objects are supported currently. We could insert a precondition check for that. http://gerrit.cloudera.org:8080/#/c/19304/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/19304/4/fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java@441 PS4, Line 441: TestHiveUdf(UDFBin.class, createText("1100100"), createBigInt(100)); We only have one Bytes test now, is it intentional? http://gerrit.cloudera.org:8080/#/c/19304/4/fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java@514 PS4, Line 514: TestGenericUdf(createBoolean(true), Can we also test a function with more than one BINARY arguments? http://gerrit.cloudera.org:8080/#/c/19304/4/java/test-hive-udfs/src/main/java/org/apache/impala/TestGenericUdf.java File java/test-hive-udfs/src/main/java/org/apache/impala/TestGenericUdf.java: http://gerrit.cloudera.org:8080/#/c/19304/4/java/test-hive-udfs/src/main/java/org/apache/impala/TestGenericUdf.java@56 PS4, Line 56: first Now we return the last (see L322). I wonder if concatenation would be possible for BINARY, too? http://gerrit.cloudera.org:8080/#/c/19304/4/java/test-hive-udfs/src/main/java/org/apache/impala/TestGenericUdf.java@57 PS4, Line 57: * that argument. Not new in this patch, but we could also mention that if any argument is null, the result is also null. http://gerrit.cloudera.org:8080/#/c/19304/4/java/test-hive-udfs/src/main/java/org/apache/impala/TestGenericUdf.java@114 PS4, Line 114: last Shouldn't this be "first"? http://gerrit.cloudera.org:8080/#/c/19304/4/java/test-hive-udfs/src/main/java/org/apache/impala/TestGenericUdf.java@123 PS4, Line 123: ObjectInspector Can't we take PrimitiveObjectInspector here? No need for the cast then. http://gerrit.cloudera.org:8080/#/c/19304/4/java/test-hive-udfs/src/main/java/org/apache/impala/TestGenericUdf.java@389 PS4, Line 389: if (result == null) return null; Before this change we returned a BytesWritable without setting it, not 'null'. Also, we null-check 'result' on L391, too, which is unnecessary. http://gerrit.cloudera.org:8080/#/c/19304/4/java/test-hive-udfs/src/main/java/org/apache/impala/TestGenericUdfWithJavaReturnTypes.java File java/test-hive-udfs/src/main/java/org/apache/impala/TestGenericUdfWithJavaReturnTypes.java: http://gerrit.cloudera.org:8080/#/c/19304/4/java/test-hive-udfs/src/main/java/org/apache/impala/TestGenericUdfWithJavaReturnTypes.java@42 PS4, Line 42: * Similarly to TestGenericUdf this class also has copy in the FE. If we wrote here both places where this file is, this line wouldn't have to be different in the the versions (like with TestGenericUdf.java). That's less error prone (though a bit less informative) because a simple copy is enough to sync the files. http://gerrit.cloudera.org:8080/#/c/19304/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/19304/4/testdata/workloads/functional-query/queries/QueryTest/generic-java-udf.test@24 PS4, Line 24: #Test GenericUDF functions We could also check some of the multi-argument functions with Java Object return types. -- To view, visit http://gerrit.cloudera.org:8080/19304 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30679045d6693ebd35718b6f1a22aaa4963c1e63 Gerrit-Change-Number: 19304 Gerrit-PatchSet: 4 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Fri, 02 Dec 2022 13:45:22 +0000 Gerrit-HasComments: Yes
