Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24656 )
Change subject: IMPALA-15238: Add TPC-DS planner tests with CTE suggestions enabled ...................................................................... Patch Set 11: Code-Review+1 (2 comments) PS11: re-checked the two points from PS5, both look right to me. I went through the Java changes and the test structure rather than the 32k lines of regenerated goldens. Two small notes below, neither of them blocking. http://gerrit.cloudera.org:8080/#/c/24656/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java: http://gerrit.cloudera.org:8080/#/c/24656/11/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java@392 PS11, Line 392: ctes = ctes.stream().sorted(Comparator.comparing(RelOptUtil::toString)).toList(); Comparator.comparing() re-evaluates its key function on every comparison, so RelOptUtil.toString() runs O(n log n) times instead of once per CTE, and each call renders the whole CTE subtree. runCTEProgram() is on the normal compilation path whenever cte_threshold > 0, so would it be worth computing the string once per CTE and sorting on that - e.g. map each CTE to a (plan string, rel) pair, sort the pairs, then take the rels? Happy to leave it as is if the CTE count is expected to stay in the single digits. http://gerrit.cloudera.org:8080/#/c/24656/11/java/calcite-planner/src/test/java/org/apache/impala/calcite/planner/TpcdsCtePlannerTest.java File java/calcite-planner/src/test/java/org/apache/impala/calcite/planner/TpcdsCtePlannerTest.java: http://gerrit.cloudera.org:8080/#/c/24656/11/java/calcite-planner/src/test/java/org/apache/impala/calcite/planner/TpcdsCtePlannerTest.java@51 PS11, Line 51: * copy of {@link org.apache.impala.planner.TpcdsCpuCostPlannerTest} with few tweaks Small one: the {@link} points at the FE class org.apache.impala.planner.TpcdsCpuCostPlannerTest, while the copy seems to come from the Calcite one in this package - same admission-control setup, CALCITE planner, and copyClasspathFileToTemp() below uses that class's loader. Could we link the Calcite version so a reader lands on the file this was actually derived from? -- To view, visit http://gerrit.cloudera.org:8080/24656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifdf22c054d94e35ccbd9edae89ce8e1606194155 Gerrit-Change-Number: 24656 Gerrit-PatchSet: 11 Gerrit-Owner: Stamatis Zampetakis <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Tue, 18 Aug 2026 10:23:45 +0000 Gerrit-HasComments: Yes
