Add HBO support for CTE materialization #22606

feilong-liu · 2024-04-24T21:17:13Z

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

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Enable HBO for CTE materialized query :pr:`22606`

feilong-liu · 2024-04-24T21:18:03Z

presto-main/src/main/java/com/facebook/presto/sql/planner/CanonicalPlanGenerator.java

+    public Optional<PlanNode> visitSequence(SequenceNode node, Context context)
+    {
+        node.getCteProducers().forEach(x -> x.accept(this, context));
+        return node.getPrimarySource().accept(this, context);


For sequence node, just return the canonicalized primary source

feilong-liu · 2024-04-24T21:18:34Z

presto-main/src/main/java/com/facebook/presto/sql/planner/CanonicalPlanGenerator.java

+    @Override
+    public Optional<PlanNode> visitCteProducer(CteProducerNode node, Context context)
+    {
+        return node.getSource().accept(this, context);


For cte producer, canonicalize its source

feilong-liu · 2024-04-24T21:18:49Z

presto-main/src/main/java/com/facebook/presto/sql/planner/CanonicalPlanGenerator.java

+    @Override
+    public Optional<PlanNode> visitCteConsumer(CteConsumerNode node, Context context)
+    {
+        return node.getOriginalSource().accept(this, context);


Canonicalize original source for cte consumer

jaystarshot · 2024-04-24T21:22:16Z

presto-main/src/main/java/com/facebook/presto/sql/planner/CanonicalPlanGenerator.java

+    @Override
+    public Optional<PlanNode> visitSequence(SequenceNode node, Context context)
+    {
+        node.getCteProducers().forEach(x -> x.accept(this, context));


Do we need to visit producers?

Yes, it's to trigger canonicalization of plan nodes under the cte producer, which will also need HBO to optimize the subquery.

jaystarshot

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.

ClarenceThreepwood · 2024-05-08T23:03:28Z

presto-main/src/main/java/com/facebook/presto/sql/planner/CanonicalPlanGenerator.java

@@ -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)


Add a test case where the sequence node is also canonicalized?

The end to end test in testHistoryBasedStatsCalculatorCTE should suffice to make sure that it works.

I was initially thinking that since the matching happens at the PlanFragment level, the Sequence would have been rewritten anyway (fyi - removing this method does not cause the test testHistoryBasedStatsCalculatorCTE to fail). But leaving it in is probably safe and future-proof

jaystarshot · 2024-05-11T22:34:09Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveHistoryBasedStatsTracking.java

+                .setSystemProperty(CTE_PARTITIONING_PROVIDER_CATALOG, "hive")
+                .build();
+        // CBO Statistics
+        assertPlan(cteMaterialization, sql, anyTree(node(ProjectNode.class, anyTree(any())).withOutputRowCount(Double.NaN)));


@feilong-liu will this also work if the cte materialization was off in the first execution and then on in the second?

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.

No, both cases are not supported. History of CTE materialization runs only apply to CTE materialization runs and non cte materialization runs only apply to non cte materialization runs.

The challenge of support history share between CTE and non CTE runs is that, for non CTE materialization runs, the CTE will be inlined and expanded in query plan, which will have different variable name and plan node ids, and may also go through different optimizations which results in different plans. In order to match the plan hash of CTEs between CTE and non CTE runs, we need to carefully design and change the HBO related plan caononicalization strategy and code, which is non-trival.

So I prefer to first enable HBO for history to be used within CTE matrilizations queries. Otherwise HBO will be barely available (as the CTE related node is not canonicalized and will return empty in hash, which will lead to all other operators which has these CTE nodes as child also return empty in hash)

Sg, I have fixed the planNodeIds and now they are canonicalized as expected in an earlier commit.
For the different variable names, I believe we can still use the underlying plan statistics available in cteConsumer/producer (original node HBO stats). These statistics will be accessible if the previous query was run in non-CTE materialized mode. However linking with the execution stats is unclear

steveburnett · 2024-05-13T14:08:56Z

Suggest minor change in release note entry to include the PR number:

== RELEASE NOTES ==

General Changes
* Enable HBO for CTE materialized query :pr:`22606`

feilong-liu requested review from jaystarshot and a team as code owners April 24, 2024 21:17

feilong-liu requested review from presto-oss, pranjalssh and rschlussel April 24, 2024 21:17

feilong-liu commented Apr 24, 2024

View reviewed changes

jaystarshot reviewed Apr 24, 2024

View reviewed changes

feilong-liu force-pushed the cte_hbo branch from c78334e to 64a7207 Compare April 24, 2024 21:22

jaystarshot approved these changes Apr 24, 2024

View reviewed changes

Add changes to HBO to work for CTE materialization

822d3a2

feilong-liu force-pushed the cte_hbo branch from 64a7207 to 822d3a2 Compare April 24, 2024 22:28

jaystarshot requested a review from tdcmeehan May 7, 2024 01:34

ClarenceThreepwood reviewed May 8, 2024

View reviewed changes

jaystarshot reviewed May 11, 2024

View reviewed changes

rschlussel approved these changes May 17, 2024

View reviewed changes

feilong-liu merged commit 6af662d into prestodb:master May 17, 2024
56 checks passed

feilong-liu deleted the cte_hbo branch May 17, 2024 19:51

wanglinsong mentioned this pull request Jun 25, 2024

Add release notes for 0.288 #23079

Merged

36 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HBO support for CTE materialization #22606

Add HBO support for CTE materialization #22606

feilong-liu commented Apr 24, 2024 •

edited

Loading

feilong-liu Apr 24, 2024

feilong-liu Apr 24, 2024

feilong-liu Apr 24, 2024

jaystarshot Apr 24, 2024

feilong-liu Apr 24, 2024

jaystarshot left a comment •

edited

Loading

ClarenceThreepwood May 8, 2024

feilong-liu May 10, 2024

ClarenceThreepwood May 14, 2024

jaystarshot May 11, 2024

jaystarshot May 11, 2024

feilong-liu May 17, 2024

jaystarshot May 17, 2024

steveburnett commented May 13, 2024

Add HBO support for CTE materialization #22606

Add HBO support for CTE materialization #22606

Conversation

feilong-liu commented Apr 24, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaystarshot left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett commented May 13, 2024

feilong-liu commented Apr 24, 2024 •

edited

Loading

jaystarshot left a comment •

edited

Loading