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 10: (9 comments) http://gerrit.cloudera.org:8080/#/c/23317/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java: http://gerrit.cloudera.org:8080/#/c/23317/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java@234 PS10, Line 234: boolean isExprOpSlotRef(Expr expr) { This function is not used anywhere. 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: > Heh, in my mind, I was thinking that 1 is a greater ranking than 5, but fro Done 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: ll exprs w > Done Done 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: // returns the Select node on top of the Analytic Node. The Calcite planner plan > Unlike the original Impala planner, unassignedConjuncts is not used across Done 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: createAnalyticExpr(rexBuilder, over, ctx, inpu > Nah, I meant that while they can be duplicated with the list of projects, t Done 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: } > This is the exact type scenario for which the uniqueAnalyticExprs is checki Done 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: > Done Done http://gerrit.cloudera.org:8080/#/c/23317/10/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/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java@255 PS10, Line 255: f it explicitly contains nulls first This is saying the same thing as the previous statement. Should it be 'does NOT contain NULLS LAST' ? http://gerrit.cloudera.org:8080/#/c/23317/10/testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q02.test File testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q02.test: http://gerrit.cloudera.org:8080/#/c/23317/10/testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q02.test@276 PS10, Line 276: | | build expressions: subtract(tpcds_partitioned_parquet_snap.date_dim.d_week_seq, 53) This seems suspicious .. why would this patch cause the build expr to change ? The query does not even have a rank or limit, so I would not have expected it to change q02 plan at all. -- 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: 10 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-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Mon, 06 Oct 2025 16:13:55 +0000 Gerrit-HasComments: Yes
