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 11: (7 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 doing the refactoring as a preceding patch, assuming you have enough clarity on what needs to be refactored. The approach below seems fine too, we'll just want to do the refactoring eventually anyway. 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". 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 any that are unused? Side note: we really need some sort of Java linter/analyzer in our pre-commit checks. 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? 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? 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. 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@58 PS11, Line 58: AuthorizationFactory authzFactory, nit: Alignment here looks off. -- 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-Comment-Date: Wed, 10 Jul 2024 00:01:23 +0000 Gerrit-HasComments: Yes
