Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21357 )
Change subject: IMPALA-12935: First pass on Calcite planner functions ...................................................................... Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java: http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java@61 PS7, Line 61: abstract public class FunctionSignature { Impala has logic for this type of matching as part of its Function class. Is there a way we could use that logic? Why or why not? http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionSignature.java@210 PS7, Line 210: It is possible for signatures to be equal when they have : // a different number of arguments. For this reason, we will return the same : // hashcode based only on the first argument, if it exists. This will result : // in a couple extra collisions, but this cost should be small. Ok, I get this now. This is about vararg functions. It's ok to hash the first arg, because every vararg function has at least one non-vararg argument. http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java: http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java@50 PS7, Line 50: public class ImpalaOperatorTable extends ReflectiveSqlOperatorTable { If I'm understanding the Calcite code, this is primarily for sets of functions that rarely change. How do UDFs fit into that? How does the session's current database interact with function lookups? Have you considered reusing the existing Impala structures for looking up functions? What does using the Calcite structure get us? My mental model here is that function resolution should basically produce the same results for Calcite vs regular Impala. Is that accurate or inaccurate? http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java: http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java@125 PS7, Line 125: TPrimitiveType primitiveType = impalaType.getPrimitiveType().toThrift(); : Type normalizedImpalaType = getImpalaType(primitiveType); Nit: It may not be necessary to convert to TPrimitiveType. We have locations in code that do a switch directly on PrimitiveType. http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java@200 PS7, Line 200: -1 Nit: For these -1 precision values, can you use RelDataType::PRECISION_NOT_SPECIFIED instead? http://gerrit.cloudera.org:8080/#/c/21357/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java@320 PS7, Line 320: public static RelDataType createRelDataType(Type impalaType) { What is the distinction between getRelDataType(Type impalaType) and createRelDataType(Type impalaType)? It looks like one is normalized and the other isn't for decimal? Let's try to express that in the names so we can tell them apart. -- To view, visit http://gerrit.cloudera.org:8080/21357 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2dd4e402d69ee10547abeeafe893164ffd789b88 Gerrit-Change-Number: 21357 Gerrit-PatchSet: 7 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Tue, 07 May 2024 04:12:39 +0000 Gerrit-HasComments: Yes
