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 15: (6 comments) I have barely started, but here are a few first comment 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; : } Are there any expressions in the sort that need to be handled? e.g. the sort expression? 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: Additional 'cast' RexCalls will be added into the Project : * and the AggregateCall will point into these input columns. Does this have any significance to the tuple layout or is it purely logical? http://gerrit.cloudera.org:8080/#/c/21335/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java@335 PS15, Line 335: getTighterDataType I get the meaning that it is avoiding unnecessary widening, but the name is a bit weird because it is picking the wider type (e.g. getTighterDataType(TINYINT, INT) would choose INT). Something like getCompatibleDataType() might be better. http://gerrit.cloudera.org:8080/#/c/21335/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java@396 PS15, Line 396: * compare 2 datatypes and return a datatype that > line has trailing whitespace Also, this comment looks unfinished http://gerrit.cloudera.org:8080/#/c/21335/15/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/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java@61 PS15, Line 61: Impala-XXX Fill this in when you get a chance http://gerrit.cloudera.org:8080/#/c/21335/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java@92 PS15, Line 92: // Eliminate the "Sarg" function which is unknown to Impala. : // TODO: this is kinda hacky. It would be better if Impala can handle this : // directly, so this needs investigation. My understanding is that sargability is related to the ability to use indexes. Impala currently doesn't use indexes, but we can look into whether Impala should understand this. -- 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: 15 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-Comment-Date: Fri, 06 Sep 2024 22:51:19 +0000 Gerrit-HasComments: Yes
