Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22031 )
Change subject: IMPALA-13513: Support decode function ...................................................................... Patch Set 5: (7 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 operands are search > typo: operands Done http://gerrit.cloudera.org:8080/#/c/22031/4//COMMIT_MSG@12 PS4, Line 12: need > need? Done 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<>(operands.size()); > nit: this could initialize capacity to match operands Done 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 hasElse = (argTypes.size() % 2 == 0); > nit: Could this be determined at the end by comparing newOperands to operan Done 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' Done 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 Done 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: RexBuilder rexBuilder = > nit: unnecessary newline Done -- 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: 5 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-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Fri, 08 Nov 2024 22:22:35 +0000 Gerrit-HasComments: Yes
