Add HBO support for CTE materialization #22606
Conversation
| public Optional<PlanNode> visitSequence(SequenceNode node, Context context) | ||
| { | ||
| node.getCteProducers().forEach(x -> x.accept(this, context)); | ||
| return node.getPrimarySource().accept(this, context); |
There was a problem hiding this comment.
For sequence node, just return the canonicalized primary source
| @Override | ||
| public Optional<PlanNode> visitCteProducer(CteProducerNode node, Context context) | ||
| { | ||
| return node.getSource().accept(this, context); |
There was a problem hiding this comment.
For cte producer, canonicalize its source
| @Override | ||
| public Optional<PlanNode> visitCteConsumer(CteConsumerNode node, Context context) | ||
| { | ||
| return node.getOriginalSource().accept(this, context); |
There was a problem hiding this comment.
Canonicalize original source for cte consumer
| @Override | ||
| public Optional<PlanNode> visitSequence(SequenceNode node, Context context) | ||
| { | ||
| node.getCteProducers().forEach(x -> x.accept(this, context)); |
There was a problem hiding this comment.
Do we need to visit producers?
There was a problem hiding this comment.
Yes, it's to trigger canonicalization of plan nodes under the cte producer, which will also need HBO to optimize the subquery.
There was a problem hiding this comment.
Looks good, just note that the stats equivalent node of the cteProducer and cteConsumer is the actual source too, I think this stats equivalent node is also passed to the table writers and table scan nodes in the physical optimizer and so tracking should also work but the flow is not entirely clear.
| @@ -830,6 +833,25 @@ public Optional<PlanNode> visitAggregation(AggregationNode node, Context context | |||
| return Optional.of(canonicalPlan); | |||
| } | |||
|
|
|||
| @Override | |||
| public Optional<PlanNode> visitSequence(SequenceNode node, Context context) | |||
There was a problem hiding this comment.
Add a test case where the sequence node is also canonicalized?
There was a problem hiding this comment.
The end to end test in testHistoryBasedStatsCalculatorCTE should suffice to make sure that it works.
| .setSystemProperty(CTE_PARTITIONING_PROVIDER_CATALOG, "hive") | ||
| .build(); | ||
| // CBO Statistics | ||
| assertPlan(cteMaterialization, sql, anyTree(node(ProjectNode.class, anyTree(any())).withOutputRowCount(Double.NaN))); |
There was a problem hiding this comment.
@feilong-liu will this also work if the cte materialization was off in the first execution and then on in the second?
There was a problem hiding this comment.
Also the opposite case, if cte materialization was on for the first run and then off again. I am a little confused and not sure it will work as expected in all these cases.
Description
The three nodes, Sequence node, cte producer node, and cte consumer node are not handled in Canonicalization for HBO. It will make HBO not working for the query. This PR adds plan canonicalization for it.
Motivation and Context
To have HBO work for CTE materialized query
Impact
HBO work for CTE materialized query
Test Plan
Unit test
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.