Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/23317 )
Change subject: IMPALA-14115: Calcite planner: Added top-n analytic PlanNode optimization. ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/23317/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23317/2//COMMIT_MSG@26 PS2, Line 26: This has been tested against the test_limit_pushdown_analytic.py file The e2e tests is good but it does not verify if the top-n was created as expected. Can we add a few planner tests for this by re-using existing planner tests in testdata/workloads/functional-planner/queries/PlannerTest/limit-pushdown-analytic.test and analytic-rank-pushdown.test. Besides the positive tests, there are also negative tests where we want to ensure the limit pushdown is not applied. http://gerrit.cloudera.org:8080/#/c/23317/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java: http://gerrit.cloudera.org:8080/#/c/23317/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java@153 PS2, Line 153: (e.g. ranking > 5) The optimization is applicable for < or <= type filters, not for > because we want the top N rows. http://gerrit.cloudera.org:8080/#/c/23317/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java@161 PS2, Line 161: not a rank nit: can we mention both RANK and ROW_NUMBER ? http://gerrit.cloudera.org:8080/#/c/23317/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java@172 PS2, Line 172: simplifiedAnalyzer.setUnassignedConjuncts(converter.getImpalaConjuncts()); This will overwrite the unassigned conjuncts .. is this the desired behavior ? I would think that the new conjuncts should be added to the unassigned, not overwrite it or at the least, save the previous unassigned conjuncts such that they can be restored later. http://gerrit.cloudera.org:8080/#/c/23317/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java@443 PS2, Line 443: AnalyticExpr must be unique within the AnalyticExpr This comment seems either incomplete or incorrect. Did you mean unique within list of projects ? http://gerrit.cloudera.org:8080/#/c/23317/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java@453 PS2, Line 453: if (!uniqueAnalyticExprs.contains(analyticExpr)) { If we have 2 OVER exprs which are identical but have different aliases, would this cause any issues with the way the uniqueness is being tracked ? e.g SELECT rank() OVER (order by c1) as rank_1, rank() OVER (order by c1) as rank_2 FROM .. http://gerrit.cloudera.org:8080/#/c/23317/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java@493 PS2, Line 493: SqlKind.RANK nit: or a SqlKind.ROW_NUMBER -- To view, visit http://gerrit.cloudera.org:8080/23317 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie6fa6781db56771b13b0cf49bd236f776016bf8d Gerrit-Change-Number: 23317 Gerrit-PatchSet: 2 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Sun, 28 Sep 2025 21:31:43 +0000 Gerrit-HasComments: Yes
