Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/18020 )
Change subject: IMPALA-10997: Refactor Java Hive UDF code. ...................................................................... Patch Set 13: Code-Review+1 (6 comments) great work, only nits! http://gerrit.cloudera.org:8080/#/c/18020/10/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaClass.java File fe/src/main/java/org/apache/impala/hive/executor/HiveJavaClass.java: http://gerrit.cloudera.org:8080/#/c/18020/10/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaClass.java@100 PS10, Line 100: could be static http://gerrit.cloudera.org:8080/#/c/18020/10/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaClass.java@116 PS10, Line 116: could be static http://gerrit.cloudera.org:8080/#/c/18020/13/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java File fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java: http://gerrit.cloudera.org:8080/#/c/18020/13/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java@88 PS13, Line 88: functions " + "are supported." nit: could be merged to 1 string http://gerrit.cloudera.org:8080/#/c/18020/13/fe/src/main/java/org/apache/impala/hive/executor/HiveUdfExecutorLegacy.java File fe/src/main/java/org/apache/impala/hive/executor/HiveUdfExecutorLegacy.java: http://gerrit.cloudera.org:8080/#/c/18020/13/fe/src/main/java/org/apache/impala/hive/executor/HiveUdfExecutorLegacy.java@122 PS13, Line 122: return method_.invoke(udf_, inputArgs_); I think that we could call evaluate much more efficiently with MethodHandle + invokeExact I would prefer to do this in a separate commit, just adding the comment to not forget it. http://gerrit.cloudera.org:8080/#/c/18020/13/fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java File fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java: http://gerrit.cloudera.org:8080/#/c/18020/13/fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java@144 PS13, Line 144: public void extractFailNoEvaluateMethodsTest() { nit: in the FE tests I know we always prefix the actual test functions with "test" an example: https://github.com/apache/impala/blob/master/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java#L48 http://gerrit.cloudera.org:8080/#/c/18020/13/fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java@208 PS13, Line 208: funciton typo -- To view, visit http://gerrit.cloudera.org:8080/18020 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc9572e15fbed1876412159b99dddd3fb4d37174 Gerrit-Change-Number: 18020 Gerrit-PatchSet: 13 Gerrit-Owner: Steve Carlin <scar...@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: Steve Carlin <scar...@cloudera.com> Gerrit-Comment-Date: Fri, 28 Jan 2022 17:15:06 +0000 Gerrit-HasComments: Yes
