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 15: (4 comments) http://gerrit.cloudera.org:8080/#/c/21238/15/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/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAggRel.java@143 PS15, Line 143: context.ctx_.getRootAnalyzer() > Nit: I think it would help maintain continuity if we use "simplifiedAnalyze Done http://gerrit.cloudera.org:8080/#/c/21238/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAggRel.java@238 PS15, Line 238: if (!multiAggInfo.getIsGroupingSet()) { : firstPhaseAgg.unsetNeedsFinalize(); : } > The logic here is slightly different from the logic in SingleNodePlanner::c Ok, changed it to match. Not sure why it is this way since I didn't put a comment when this code was added. But if something is broken later, we can file a specific bug and it will be tracked better. http://gerrit.cloudera.org:8080/#/c/21238/15/testdata/workloads/functional-query/queries/QueryTest/calcite.test File testdata/workloads/functional-query/queries/QueryTest/calcite.test: http://gerrit.cloudera.org:8080/#/c/21238/15/testdata/workloads/functional-query/queries/QueryTest/calcite.test@300 PS15, Line 300: ---- QUERY > Is it possible to do a test case that uses a cardinality check? Looking thr Yeah, pretty sure you need a join to do this. This is tested in later commits. http://gerrit.cloudera.org:8080/#/c/21238/15/testdata/workloads/functional-query/queries/QueryTest/calcite.test@362 PS15, Line 362: ==== > I was running some of the test cases from grouping-sets.test and I get an " The NPE is because the fn passed in is null. I put in a Preconditions check in the caller. Specifically, I'm pretty sure grouping_id has to be handled in a separate way, and I have this in a commit to be published later. -- 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: 15 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: Tue, 27 Aug 2024 14:06:07 +0000 Gerrit-HasComments: Yes
