Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19177 )
Change subject: WIP IMPALA-11718: Improve support for Hive Generic UDFs ...................................................................... Patch Set 6: (17 comments) http://gerrit.cloudera.org:8080/#/c/19177/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19177/6//COMMIT_MSG@9 PS6, Line 9: s Nit: provide http://gerrit.cloudera.org:8080/#/c/19177/6//COMMIT_MSG@10 PS6, Line 10: get Nit: gets http://gerrit.cloudera.org:8080/#/c/19177/6//COMMIT_MSG@18 PS6, Line 18: i Nit: "a" in signature http://gerrit.cloudera.org:8080/#/c/19177/6//COMMIT_MSG@23 PS6, Line 23: e Nit: analysis? http://gerrit.cloudera.org:8080/#/c/19177/6//COMMIT_MSG@30 PS6, Line 30: e Nit: analysis http://gerrit.cloudera.org:8080/#/c/19177/6/common/thrift/Types.thrift File common/thrift/Types.thrift: http://gerrit.cloudera.org:8080/#/c/19177/6/common/thrift/Types.thrift@266 PS6, Line 266: its Nit: we could correct it to "it's". http://gerrit.cloudera.org:8080/#/c/19177/6/fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java File fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java: http://gerrit.cloudera.org:8080/#/c/19177/6/fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java@42 PS6, Line 42: ScalarFunction We could consider making GenericScalarFunction and ConcreteScalarFunction separate classes, although that would make the patch bigger. In that case, it could be expressed in the type system that the result of getSpecializedFunction() is a different kind of scalar function than the "this" argument (concrete vs generic). I'm not sure it's worth the extra complexity but we can at least consider it. http://gerrit.cloudera.org:8080/#/c/19177/6/fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java@71 PS6, Line 71: Type.INVALID Is Type.INVALID a good choice here for generic functions? We may not have many alternatives, but for example in toSql() on L244 it would be misleading if we printed "INVALID" instead of something like "GENERIC". http://gerrit.cloudera.org:8080/#/c/19177/6/fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java File fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java: http://gerrit.cloudera.org:8080/#/c/19177/6/fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java@96 PS6, Line 96: retType_ = retType; We could add a Preconditions check here that parameterTypes != null - it could happen if calling the constructor on L154 with a retType but no parameterTypes. http://gerrit.cloudera.org:8080/#/c/19177/6/fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java@142 PS6, Line 142: // Create specialized version from generic function. The comment is the same as for the function on L136, we could clarify the difference. This could also be a non-static function like ScalarFunction.getSpecializedFunction(). http://gerrit.cloudera.org:8080/#/c/19177/6/fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java@173 PS6, Line 173: Preconditions.checkState(!fullSignatureKnown_); Can't we return a non-generic ScalarFunction if this function is fully specialised? http://gerrit.cloudera.org:8080/#/c/19177/6/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java File fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java: http://gerrit.cloudera.org:8080/#/c/19177/6/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java@55 PS6, Line 55: toHiveFunction Not introduced in this patch, but doesn't this function belong more to ScalarFunction? Shouldn't it be moved there? It has no connection with HiveJavaFunction. http://gerrit.cloudera.org:8080/#/c/19177/6/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java@63 PS6, Line 63: public Is this function called anywhere else? Could it be private? Or do you think it may be useful later? http://gerrit.cloudera.org:8080/#/c/19177/6/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/19177/6/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java@149 PS6, Line 149: public Do we use it elsewhere, can it be private? http://gerrit.cloudera.org:8080/#/c/19177/6/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/19177/6/fe/src/main/java/org/apache/impala/hive/executor/JavaUdfDataType.java@52 PS6, Line 52: // should be BINARY Should we add a TODO and/or Jira about it? http://gerrit.cloudera.org:8080/#/c/19177/6/fe/src/main/java/org/apache/impala/util/FunctionUtils.java File fe/src/main/java/org/apache/impala/util/FunctionUtils.java: http://gerrit.cloudera.org:8080/#/c/19177/6/fe/src/main/java/org/apache/impala/util/FunctionUtils.java@115 PS6, Line 115: return ((ScalarFunction)f).getSpecializedFunction(desc.getArgs()); Preconditions check that f is a ScalarFunction. http://gerrit.cloudera.org:8080/#/c/19177/6/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/19177/6/java/test-hive-udfs/src/main/java/org/apache/impala/TestGenericUdf.java@108 PS6, Line 108: last First? We get arguments[0]. -- To view, visit http://gerrit.cloudera.org:8080/19177 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I692d402784fa91d1256f5d57fa16ac49728447c8 Gerrit-Change-Number: 19177 Gerrit-PatchSet: 6 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Tue, 06 Dec 2022 13:34:00 +0000 Gerrit-HasComments: Yes
