Joe McDonnell 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 "simplifiedAnalyzer" as the argument here. 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::createAggregationPlan(). The logic in SingleNodePlanner calls unsetNeedsFinalize() in the section for hasSecondPhase(), whereas this calls it when there is no grouping set. I don't have the expertise to judge whether that difference matters, but it would be nice if this logic stayed close to SingleNodePlanner. 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 through our test cases, the ones with a cardinality check tend to have a join, so maybe it isn't feasible at this point. 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 "InternalException: null" for the following query: select grouping_id(int_col, string_col), grouping(int_col), grouping(string_col), int_col, string_col, count(distinct tinyint_col), count(distinct timestamp_col) from alltypestiny group by int_col, string_col I'm not sure if we expect this to work, but here is the stack trace: 5d82e00000000] Exception: java.lang.NullPointerException I0814 13:20:24.749795 3646487 CalciteJniFrontend.java:143] dd40b29bd1dc3da5:fd55d82e00000000] Stack Trace:java.lang.NullPointerException at org.apache.impala.calcite.functions.AnalyzedFunctionCallExpr.<init>(AnalyzedFunctionCallExpr.java:62) at org.apache.impala.calcite.rel.node.ImpalaAggRel.getAggregateExprs(ImpalaAggRel.java:284) at org.apache.impala.calcite.rel.node.ImpalaAggRel.getPlanNode(ImpalaAggRel.java:105) at org.apache.impala.calcite.rel.node.ImpalaProjectRel.getChildPlanNode(ImpalaProjectRel.java:111) at org.apache.impala.calcite.rel.node.ImpalaProjectRel.getPlanNode(ImpalaProjectRel.java:71) at org.apache.impala.calcite.service.CalcitePhysPlanCreator.create(CalcitePhysPlanCreator.java:68) at org.apache.impala.calcite.service.CalciteJniFrontend.createExecRequest(CalciteJniFrontend.java:125) I0814 13:20:24.749838 3646487 jni-util.cc:321] dd40b29bd1dc3da5:fd55d82e00000000] org.apache.impala.common.InternalException at org.apache.impala.calcite.service.CalciteJniFrontend.createExecRequest(CalciteJniFrontend.java:144) -- 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: Wed, 14 Aug 2024 23:58:28 +0000 Gerrit-HasComments: Yes
