Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21238 )
Change subject: IMPALA-12964: Implement basic aggregation in the Calcite planner ...................................................................... Patch Set 13: (15 comments) http://gerrit.cloudera.org:8080/#/c/21238/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21238/11//COMMIT_MSG@21 PS11, Line 21: 2) Avoid refactoring in the first major iteration of the Calcite planner. > Ack Done http://gerrit.cloudera.org:8080/#/c/21238/11//COMMIT_MSG@28 PS11, Line 28: 1) "Having" filter conjuncts are going to be "unassigned conjuncts". > Done Ack http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAggRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAggRel.java: http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAggRel.java@21 PS11, Line 21: import com.google.common.base.Preconditions; > Incorrect assumption! Done http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAggRel.java@108 PS11, Line 108: getGroupingSets(simplifiedAnalyzer, inputWithExprs.outputExprs_); > Done Ack http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAggRel.java@141 PS11, Line 141: simplifiedAnalyzer.setUnassignedConjuncts(converter.getImpalaConjuncts()); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAggRel.java@159 PS11, Line 159: private List<Expr> getGroupingExprs(List<Expr> inputExprs) { > line too long (95 > 90) Done http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAggRel.java@184 PS11, Line 184: if (groupSets.size() == 0) { > line too long (109 > 90) Done http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAggRel.java@256 PS11, Line 256: > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAggRel.java@329 PS11, Line 329: return new NodeWithExprs(cardinalityCheckNode, outputExprs); > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAggRel.java@339 PS11, Line 339: /** > So Aggregate requires copy to be defined because it is abstract. But I ran I don't have much of an opinion. Throwing an exception would let us know if it were ever needed, but if you implemented it right already it would just work. http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAggRel.java@340 PS11, Line 340: * Create the output expressions for the ImpalaAggregateRel. > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAggRel.java@354 PS11, Line 354: for (Expr e : groupingExprs) { > line too long (98 > 90) Done http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java: http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaPlanRel.java@35 PS11, Line 35: public enum RelNodeType { > Done Ack http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/util/SimplifiedAnalyzer.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/util/SimplifiedAnalyzer.java: http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/util/SimplifiedAnalyzer.java@48 PS11, Line 48: TQueryCtx queryCtx, AuthorizationFactory authzFactory, > line too long (93 > 90) Still needed. http://gerrit.cloudera.org:8080/#/c/21238/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/util/SimplifiedAnalyzer.java@58 PS11, Line 58: public List<Expr> getBoundPredicates(TupleId destTid, Set<SlotId> ignoreSlots, > nit: Alignment here looks off. Done -- To view, visit http://gerrit.cloudera.org:8080/21238 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacf0de8ba11f0d31d73d624f0c9a91db9997cfd5 Gerrit-Change-Number: 21238 Gerrit-PatchSet: 13 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[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: Wed, 10 Jul 2024 22:13:57 +0000 Gerrit-HasComments: Yes
