Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21565 )
Change subject: IMPALA-13197: Implement Analytic Exprs for Calcite ...................................................................... Patch Set 7: (4 comments) Gonna address the crash and some other failures in a followup commit to keep this one a little simpler. http://gerrit.cloudera.org:8080/#/c/21565/7/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/21565/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java@116 PS7, Line 116: > nit: gerrit highlights some extra whitespace in a few places that would be Done http://gerrit.cloudera.org:8080/#/c/21565/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java: http://gerrit.cloudera.org:8080/#/c/21565/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java@179 PS7, Line 179: private AnalyticExpr getAnalyticExpr(RexOver rexOver, PlannerContext ctx, ImpalaPlanRel inputRel, > This line is too long. Done http://gerrit.cloudera.org:8080/#/c/21565/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java@205 PS7, Line 205: if (CollectionUtils.isNotEmpty(rexWindow.orderKeys)) { > We need this check because orderKeys could be null? Yeah, not sure why I coded it as isNotEmpty. Just changed this so it's looking for null now since the for loop handles the case when it's a zero sized list. http://gerrit.cloudera.org:8080/#/c/21565/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java@295 PS7, Line 295: ctx > PlannerContext ctx, do not need? Done -- To view, visit http://gerrit.cloudera.org:8080/21565 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba5060546a7568ba0cd315f546daa78d89b1c3c5 Gerrit-Change-Number: 21565 Gerrit-PatchSet: 7 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, 04 Oct 2024 17:19:49 +0000 Gerrit-HasComments: Yes
