Steve Carlin 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 11: (6 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. > I assume this is something you and Joe talked about? I would have suggested Ack http://gerrit.cloudera.org:8080/#/c/21238/11//COMMIT_MSG@28 PS11, Line 28: 1) "Having" filter conjuncts are going to be "unassigned conjuncts". > I don't see a test case for "having". Done 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; > I assume all of these are used. Did you do a pass with an IDE to check for Incorrect assumption! I sometimes use an IDE, but not always. I just ran all changed files here through the IDE and removed unused imports 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: (SimplifiedAnalyzer) context.ctx_.getRootAnalyzer(); > Maybe a Precondition for this cast? 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: @Override > When is copy used? Is it a requirement for Aggregate? So Aggregate requires copy to be defined because it is abstract. But I ran it through the test environment and it doesn't seem to be called. So then I guess the question is: Do I leave the code as/is? Or do I throw a "not implemented" RuntimeException? Not sure which is better. 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: AGG, > Can we spell this out? I don't see much value in truncating it. 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: 11 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:02:01 +0000 Gerrit-HasComments: Yes
