Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22091 )
Change subject: IMPALA-13531: Calcite CTE frontend ...................................................................... Patch Set 31: (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::REGULAR) \ Since "planner" is advanced, should this be advanced as well? Should we change planner to "regular"? 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: // Occurence threshold for using CTEs. CTEs are only used if occurences are greater nit: occurrence 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: // Collect CTE result expressions for ScalarExpr I'm curious if the substituteList call is needed. I know the substitute list comes into play a lot with the old planner as it traverses up and down the PlanNode stack. But a lot of this is avoided in the Calcite planner. 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 setting the first element to null? 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.cteProducers_ = new HashMap<>(builder.cteProducers_); Optional: this way is fine, but I usually prefer ImmutableMap here. The Builder can contain an ImmutableMap.Builder and this can just be set with a ".build()" 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: public Double getDistinctRowCount(ImpalaCTEConsumer rel, RelMetadataQuery mq, In an earlier code review, ImpalaCTEConsumer extended TableScan and I see this has changed. If we need an override here, do we need it in other places like RowCount? Or can we remove this because it isn't called? 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: implementing this for RelSubset doesn't seem like the right option. I think Let's file a Jira for this, costing should probably be thought out more. 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: if (referenceThreshold < 0) { In the RemoveInfrequent rule, the exception is thrown if it is <= 0. Should this be <= 0 as well? -- 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: 31 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 16:52:19 +0000 Gerrit-HasComments: Yes
