Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21335 )
Change subject: IMPALA-13022: Added infrastructure for implicit casting of functions ...................................................................... Patch Set 18: (5 comments) This is making sense to me, a few small comments http://gerrit.cloudera.org:8080/#/c/21335/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java: http://gerrit.cloudera.org:8080/#/c/21335/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java@200 PS15, Line 200: private static RelNode processSortNode(RelNode relNode, List<RelNode> inputs, : RexBuilder rexBuilder, boolean isInputChanged) { : return isInputChanged ? relNode.copy(relNode.getTraitSet(), inputs) : relNode; : } > The sort expressions just consist of a number within the sort. If there is Ack http://gerrit.cloudera.org:8080/#/c/21335/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java@211 PS15, Line 211: regate. Additional 'cast' RexCalls will be added into the Project : * and the AggregateCall will point into these input columns. > I think I overlooked this one. This will affect the layout. I'm unclear about how projects translate down to tuple layout. I feel like projects don't always need to impact tuple layout. For projects that are functions of existing columns, that expression can be evaluated in whatever node is using that expression. If we scan a table with column X and then sort by abs(X), we can evaluate that abs(X) in the sort node itself. The tuple can just be X, even though there is a Calcite Project RelNode with abs(X). It seems like this could be handled in the translation from Calcite down to Impala PlanNodes. In other words, we may not need to complicate the Calcite side of the equation. http://gerrit.cloudera.org:8080/#/c/21335/18/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java: http://gerrit.cloudera.org:8080/#/c/21335/18/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java@539 PS18, Line 539: The last "false" parameter makes sure that we : // are not looking for an exact match, just one where type conversion can : // be done. This no longer applies now that we use getSupertypeFunction() http://gerrit.cloudera.org:8080/#/c/21335/18/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java: http://gerrit.cloudera.org:8080/#/c/21335/18/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java@201 PS18, Line 201: alter Nit: alternate http://gerrit.cloudera.org:8080/#/c/21335/18/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/ImplicitTypeChecker.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/ImplicitTypeChecker.java: http://gerrit.cloudera.org:8080/#/c/21335/18/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/ImplicitTypeChecker.java@42 PS18, Line 42: private static Set<Pair<SqlTypeName, SqlTypeName>> SUPPORTED_IMPLICIT_CASTS = So, we have this table for Calcite types. Is there some relationship to the Impala compatibility tables that we could document? i.e. Is this the same as strict compatibility matrix in catalog's Type.java? Is this something that would need to change if we support the allow_unsafe_casts query option? i.e. from this commit: https://github.com/apache/impala/commit/3247cc683935f078a424a567389fa53764b300d9 -- To view, visit http://gerrit.cloudera.org:8080/21335 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I13a349673f185463276ad7ddb83f0cfc2d73218c Gerrit-Change-Number: 21335 Gerrit-PatchSet: 18 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[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: Mon, 16 Sep 2024 22:10:30 +0000 Gerrit-HasComments: Yes
