Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22031 )
Change subject: IMPALA-13513: Support decode function ...................................................................... Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/22031/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22031/4//COMMIT_MSG@10 PS4, Line 10: for supporting the decode is that some of the opernands are search typo: operands http://gerrit.cloudera.org:8080/#/c/22031/4/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/22031/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java@384 PS4, Line 384: List<RexNode> newOperands = new ArrayList<>(); nit: this could initialize capacity to match operands List<RexNode> newOperands = new ArrayList<>(operands.size()); http://gerrit.cloudera.org:8080/#/c/22031/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java@393 PS4, Line 393: boolean castedOperand = false; nit: Could this be determined at the end by comparing newOperands to operands? You need to check whether every type is identical anyway. So the return could be return operands.equals(newOperands) ? decodeCall : (RexCall) rexBuilder.makeCall(...); http://gerrit.cloudera.org:8080/#/c/22031/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java: http://gerrit.cloudera.org:8080/#/c/22031/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java@207 PS4, Line 207: // Case statement is special because the function signature only contains the I think it'd be helpful to expand this description to also address 'decode'. http://gerrit.cloudera.org:8080/#/c/22031/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaDecodeFunction.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaDecodeFunction.java: http://gerrit.cloudera.org:8080/#/c/22031/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaDecodeFunction.java@43 PS4, Line 43: * are the third parameter, the fifth parameter, the seventh, etc... Some example invocations might be nice given all the positional parameters involved. http://gerrit.cloudera.org:8080/#/c/22031/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaDecodeFunction.java@71 PS4, Line 71: nit: unnecessary newline -- To view, visit http://gerrit.cloudera.org:8080/22031 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia3b68fda7cfa14799a41428e35d5bbc5984a801a Gerrit-Change-Number: 22031 Gerrit-PatchSet: 4 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Anonymous Coward (816) Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Thu, 07 Nov 2024 22:07:59 +0000 Gerrit-HasComments: Yes
