Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22091 )
Change subject: IMPALA-13531: Calcite CTE frontend ...................................................................... Patch Set 32: (8 comments) http://gerrit.cloudera.org:8080/#/c/22091/31/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/22091/31/be/src/service/query-options.h@396 PS31, Line 396: QUERY_OPT_FN(cte_threshold, CTE_THRESHOLD, TQueryOptionLevel::ADVANCED) \ > Since "planner" is advanced, should this be advanced as well? Should we ch I've made it advanced for now, I don't want to change PLANNER in this patch. http://gerrit.cloudera.org:8080/#/c/22091/31/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/22091/31/common/thrift/ImpalaService.thrift@1103 PS31, Line 1103: // Occurrence threshold for using CTEs. CTEs are only used if occurrences are greater > nit: occurrence Done http://gerrit.cloudera.org:8080/#/c/22091/31/fe/src/main/java/org/apache/impala/planner/CTEConsumerNode.java File fe/src/main/java/org/apache/impala/planner/CTEConsumerNode.java: http://gerrit.cloudera.org:8080/#/c/22091/31/fe/src/main/java/org/apache/impala/planner/CTEConsumerNode.java@66 PS31, Line 66: @Override > I'm curious if the substituteList call is needed. Appears it's not needed; I've removed it. http://gerrit.cloudera.org:8080/#/c/22091/31/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSequence.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSequence.java: http://gerrit.cloudera.org:8080/#/c/22091/31/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSequence.java@110 PS31, Line 110: children.set(0, passthrough.planNode_); > Can we just do a "children.add(0, passthrough.planNode_)" instead of settin Slightly less optimal because it has to move the rest of the array, but we could. http://gerrit.cloudera.org:8080/#/c/22091/31/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ParentPlanRelContext.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ParentPlanRelContext.java: http://gerrit.cloudera.org:8080/#/c/22091/31/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ParentPlanRelContext.java@74 PS31, Line 74: this.parentAggregate_ = builder.parentAggregate_; > Optional: this way is fine, but I usually prefer ImmutableMap here. The Bu Done http://gerrit.cloudera.org:8080/#/c/22091/31/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaRelMdDistinctRowCount.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaRelMdDistinctRowCount.java: http://gerrit.cloudera.org:8080/#/c/22091/31/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaRelMdDistinctRowCount.java@110 PS31, Line 110: } > In an earlier code review, ImpalaCTEConsumer extended TableScan and I see t The default of getRowCount is to call estimateRowCount, which ImpalaCTEConsumer overrides. The override at https://github.com/apache/impala/blob/master/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaRelMdRowCount.java#L87 isn't necessary. The default for getDistinctRowCount assumes a selectivity, which is not the behavior we want. So I think we still need this override, but can rely on estimateRowCount for other cases. But it turns out this code wasn't used. I've fixed that. http://gerrit.cloudera.org:8080/#/c/22091/31/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaRelMdSize.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaRelMdSize.java: http://gerrit.cloudera.org:8080/#/c/22091/31/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaRelMdSize.java@51 PS31, Line 51: // TODO: IMPALA-15156: implementing this for RelSubset doesn't seem like the right > Let's file a Jira for this, costing should probably be thought out more. Done http://gerrit.cloudera.org:8080/#/c/22091/31/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/22091/31/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java@348 PS31, Line 348: // Disable CTE planning. > In the RemoveInfrequent rule, the exception is thrown if it is <= 0. Shoul Yes, done. -- To view, visit http://gerrit.cloudera.org:8080/22091 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id0840c0859d2fe25628d799a18d302cee1eb36e8 Gerrit-Change-Number: 22091 Gerrit-PatchSet: 32 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Anonymous Coward (816) Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Stamatis Zampetakis <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Wed, 08 Jul 2026 18:49:12 +0000 Gerrit-HasComments: Yes
