Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18020 )
Change subject: IMPALA-10997: Refactor Java Hive UDF code. ...................................................................... Patch Set 12: (11 comments) http://gerrit.cloudera.org:8080/#/c/18020/11/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/11/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaClass.java@41 PS11, Line 41: > Wouldn't 'HiveUdfLoader' or something like that be a better name? I think t Done http://gerrit.cloudera.org:8080/#/c/18020/11/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaClass.java@64 PS11, Line 64: : : : > This would be better as a function doc comment. The other two parameters co Done http://gerrit.cloudera.org:8080/#/c/18020/11/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java File fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java: http://gerrit.cloudera.org:8080/#/c/18020/11/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java@64 PS11, Line 64: If no retType is supplied, this will be : // null (and presumably be used for extraction of > I can see it depends on 'retType' being non-null on L76 but how is it conne Done http://gerrit.cloudera.org:8080/#/c/18020/10/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/10/fe/src/main/java/org/apache/impala/hive/executor/HiveUdfExecutorLegacy.java@69 PS10, Line 69: Returns Object returned by UDF. > Are you sure this method returns a pointer-like value? In HiveUdfExecutor, Yeah, you're right. I confused myself for a sec. Sorry about that. http://gerrit.cloudera.org:8080/#/c/18020/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/18020/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4087 PS10, Line 4087: > Nit: is this space not needed? When the strings are concatenated no additio It should be needed, right? Otherwise it will show up as "orwere" http://gerrit.cloudera.org:8080/#/c/18020/10/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6263 PS10, Line 6263: List<Partition> > I'm not familiar with this part of the code, I'm just curious why it is rem I didn't change this code but perhaps there was a bad rebase somewhere in an intermediate patchset? http://gerrit.cloudera.org:8080/#/c/18020/11/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/18020/11/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4150 PS11, Line 4150: > Is it intentional that you changed it from 'they exist'? It seems strange, I didn't change this code but perhaps there was a bad rebase somewhere in an intermediate patchset? http://gerrit.cloudera.org:8080/#/c/18020/10/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java: http://gerrit.cloudera.org:8080/#/c/18020/10/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@315 PS10, Line 315: ventsInBa > I'm really not familiar with MetastoreEventsProcessor, could you tell me th I'm doing a comparison from 10 to base here (and 11 to base) and I didn't change this code? I did see it changed in one of the intermediate patchsets so maybe there was a bad rebase at some point? http://gerrit.cloudera.org:8080/#/c/18020/11/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/11/fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java@116 PS11, Line 116: * Check that the extracted signatures match the expected signatures. > We're also checking if we have extracted something we didn't expect. Done http://gerrit.cloudera.org:8080/#/c/18020/10/fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunction.java File fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunction.java: http://gerrit.cloudera.org:8080/#/c/18020/10/fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunction.java@27 PS10, Line 27: > Ok, it's fine by me, especially if you think it may also be useful in other Done http://gerrit.cloudera.org:8080/#/c/18020/11/tests/query_test/test_udfs.py File tests/query_test/test_udfs.py: http://gerrit.cloudera.org:8080/#/c/18020/11/tests/query_test/test_udfs.py@428 PS11, Line 428: ols are checked a > Is it still non-deterministic, didn't moving invalid symbol detection to UD Done -- 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: 12 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Wed, 26 Jan 2022 04:00:46 +0000 Gerrit-HasComments: Yes
