Steve Carlin 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: (3 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. Removed 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 'doe This was actually a true statement, but the line underneath was the differentiator. It's always 'nulls first' when it is explicitly stated. But for descending, it also is 'nulls first' by default. Is it clearer now? Or should I reword it some more? 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 chang The change that caused this was the RexCallConverter.shouldFlipExpr. There was a test that was failing in the limit pushdown tests where the binary predicate needed to be normalized. Specifically, the expression was something like '11 > rnk', which works only when flipped to 'rnk < 11'. The code in the original planner used the NormalizeBinaryPredicateRule, but these rewrite rules aren't used in the Calcite planner. Hence, the "shouldFlipExpr" method. I just ran tpcds q02 with this code and it did indeed fail. By removing part of that "normalization" code, it seems to work for both cases. I don't have a full understanding of what happened here and need to investigate further, but all tpcds tests run fine now, as well as all top-n optimization tests in the CalcitePlannerTest file. -- 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 21:33:52 +0000 Gerrit-HasComments: Yes
