Add Heuristic CTE Materialization Strategy #21720

jaystarshot · 2024-01-17T18:58:36Z

This PR adds support Heuristic CTE materialization.
We have rolled this in one of our production cluster and confirmed that this is stable.
Our current testing has shown upto 50% cpu gain and > 50% less hdfs read for affected queries.
Its heuristic so there may be cases where non-ideal plans are generated.
Changed cteName member variable to cteId for cte plan nodes.
Also fixed CteInformation to include cteId so that correct references are incremented which crucial for cte materialization.

In heuristic strategy, the relational planner adds cteReferences everywhere and the LogicalCteOptimizer decides which references to implement

Fixes #21637

Motivation and Context

Impact

Test Plan

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
* Add Heuristic CTE Materialization strategy which auto materialized expensive ctes. This is configurable by setting ``cte_materialization_strategy`` to ``HEURISTIC`` or ``HEURISTIC_COMPLEX_QUERIES_ONLY``. (default ``NONE``)

jaystarshot · 2024-01-31T07:41:19Z

@feilong-liu @mlyublena Please review when you get time, thanks!

mlyublena · 2024-01-31T22:41:08Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

@@ -357,7 +357,9 @@ public boolean isAdoptingMergedPreference()
    public enum CteMaterializationStrategy
    {
        ALL, // Materialize all CTES
-        NONE // Materialize no ctes
+        NONE, // Materialize no CTES
+        HEURISTIC,


can you add comments for the new values

mlyublena · 2024-01-31T22:44:45Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

-        NONE // Materialize no ctes
+        NONE, // Materialize no CTES
+        HEURISTIC,
+        HEURISTIC_STRICT


I wonder if there is a better name than strict: perhaps HEURISTIC_COMPLEX_QUERIES_ONLY? we already have "complex query" reference in another session property (RESTRICT_HISTORY_BASED_OPTIMIZATION_TO_COMPLEX_QUERY)

mlyublena · 2024-01-31T23:07:29Z

...to-main/src/main/java/com/facebook/presto/sql/planner/optimizations/LogicalCteOptimizer.java

+            HashMap<String, CTEInformation> cteInformationMap = session.getCteInformationCollector().getCteInformationMap();
+            CTEInformation cteInfo = cteInformationMap.get(node.getCteId());
+            switch (getCteMaterializationStrategy(session)) {
+                case HEURISTIC_STRICT:


do we want to also disable materialization of VALUES nodes? so only proceed if the query is a proper join/aggregation on top of table scans

mlyublena · 2024-01-31T23:08:10Z

LGTM modulo some nits

jaystarshot · 2024-02-02T04:40:55Z

@mlyublena Thanks for your review, I have

updated the PR to include the values check
Added tests in case to assert no materialization in heuristic cases
Backported a small fix from our repo in case there is no materialization in heuristic cases (cte references were not removed)

jaystarshot · 2024-02-06T21:44:16Z

Rebased

mlyublena · 2024-02-06T22:30:34Z

presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java

@@ -1082,6 +1083,11 @@ public SystemSessionProperties(
                        "Enable pushing of filters and projections inside common table expressions.",
                        featuresConfig.getCteFilterAndProjectionPushdownEnabled(),
                        false),
+                integerProperty(
+                        CTE_HEURISTIC_REPLICATION_THRESHOLD,
+                        "Used with CTE Materialization Strategy = Heuristic. CTES are only materialized if they are used greater than or equal to this number",


nit: perhaps change comment to "if they are used more than this number of times"

...to-main/src/main/java/com/facebook/presto/sql/planner/optimizations/LogicalCteOptimizer.java

mlyublena · 2024-02-06T22:44:12Z

...to-main/src/main/java/com/facebook/presto/sql/planner/optimizations/LogicalCteOptimizer.java

-        public CteConsumerTransformer(PlanNodeIdAllocator idAllocator, VariableAllocator variableAllocator)
+        private final Session session;
+
+        private static final List<Class<? extends PlanNode>> PRECOMPUTE_PLAN_NODES = ImmutableList.of(JoinNode.class, SemiJoinNode.class, AggregationNode.class);


should we call these two COMPLEX_PLAN_NODES and COMPLEX_DATA_SOURCE_NODES or something like that to better correlate with the session property name?

pranjalssh · 2024-02-14T19:26:38Z

Please make sure failing tests are not related

jaystarshot · 2024-03-04T22:32:55Z

@mlyublena @pranjalssh Thank you for the review. However I found a test case added here wherein, the outer CTE would be materialized even if it wasn't complex if the inner CTE was complex,
hence I changed the flow a bit to accommodate this. With this i think the cbo replacement would also be easier

Now, we first determine if a CTE is complex and then perform the rewrite.
Also cleaned up some session property handling, along with some renaming in the classes for clarity.
Please re-review!
cc: @tdcmeehan

pranjalssh · 2024-03-06T17:53:49Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

@@ -364,7 +364,9 @@ public boolean isAdoptingMergedPreference()
    public enum CteMaterializationStrategy
    {
        ALL, // Materialize all CTES
-        NONE // Materialize no ctes
+        NONE, // Materialize no CTES
+        HEURISTIC, // Materialze CTES occuring  > CTE_HEURISTIC_REPLICATION_THRESHOLD


Suggested change

HEURISTIC, // Materialze CTES occuring > CTE_HEURISTIC_REPLICATION_THRESHOLD

HEURISTIC, // Materialize CTES occuring > CTE_HEURISTIC_REPLICATION_THRESHOLD

Summary: Add 2 materialization strategies Heuristic and Heuristic_Strict. In Heuristic Strategy the Ctes will only be materialized if they occured > x times. In Heuristic_strict strategy the CTE need to have a join or aggregate

github-actions · 2024-03-06T19:15:43Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 9dfc461...09db46e.

No notifications.

jaystarshot · 2024-03-06T20:03:26Z

@pranjalssh Need a review again due to the comment change

jaystarshot force-pushed the 21637 branch from 4bd8520 to c2f1924 Compare January 17, 2024 19:06

jaystarshot marked this pull request as ready for review January 25, 2024 06:00

jaystarshot requested a review from a team as a code owner January 25, 2024 06:00

jaystarshot requested review from presto-oss, tdcmeehan, feilong-liu and mlyublena January 25, 2024 06:00

jaystarshot force-pushed the 21637 branch 9 times, most recently from cfa8b25 to b3d82f7 Compare January 31, 2024 06:34

mlyublena reviewed Jan 31, 2024

View reviewed changes

mlyublena approved these changes Jan 31, 2024

View reviewed changes

jaystarshot force-pushed the 21637 branch 4 times, most recently from 93ee4fb to 693ad79 Compare February 2, 2024 04:38

jaystarshot requested a review from mlyublena February 2, 2024 04:40

jaystarshot force-pushed the 21637 branch 2 times, most recently from 7b1cc96 to db3edae Compare February 6, 2024 21:41

jaystarshot requested a review from pranjalssh February 6, 2024 21:44

jaystarshot force-pushed the 21637 branch from db3edae to 598b1f9 Compare February 6, 2024 22:12

mlyublena reviewed Feb 6, 2024

View reviewed changes

...to-main/src/main/java/com/facebook/presto/sql/planner/optimizations/LogicalCteOptimizer.java Outdated Show resolved Hide resolved

mlyublena reviewed Feb 6, 2024

View reviewed changes

mlyublena previously approved these changes Feb 6, 2024

View reviewed changes

pranjalssh previously approved these changes Feb 14, 2024

View reviewed changes

jaystarshot dismissed stale reviews from pranjalssh and mlyublena via f91f0b9 March 4, 2024 20:35

jaystarshot force-pushed the 21637 branch 2 times, most recently from f91f0b9 to df68261 Compare March 4, 2024 20:36

jaystarshot requested review from pranjalssh and mlyublena March 4, 2024 22:33

pranjalssh previously approved these changes Mar 6, 2024

View reviewed changes

jaystarshot dismissed pranjalssh’s stale review via c912e41 March 6, 2024 19:12

jaystarshot force-pushed the 21637 branch from df68261 to c912e41 Compare March 6, 2024 19:12

jaystarshot requested a review from pranjalssh March 6, 2024 19:12

Add Heuristic CTE Materialization Strategy

09db46e

Summary: Add 2 materialization strategies Heuristic and Heuristic_Strict. In Heuristic Strategy the Ctes will only be materialized if they occured > x times. In Heuristic_strict strategy the CTE need to have a join or aggregate

jaystarshot force-pushed the 21637 branch from c912e41 to 09db46e Compare March 6, 2024 19:13

tdcmeehan approved these changes Mar 6, 2024

View reviewed changes

mlyublena approved these changes Mar 6, 2024

View reviewed changes

jaystarshot merged commit 68c507d into prestodb:master Mar 6, 2024
56 checks passed

wanglinsong mentioned this pull request May 1, 2024

Add release notes for 0.287 #22647

Merged

48 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Heuristic CTE Materialization Strategy #21720

Add Heuristic CTE Materialization Strategy #21720

jaystarshot commented Jan 17, 2024 •

edited

Loading

jaystarshot commented Jan 31, 2024

mlyublena Jan 31, 2024

mlyublena Jan 31, 2024

mlyublena Jan 31, 2024

mlyublena commented Jan 31, 2024

jaystarshot commented Feb 2, 2024

jaystarshot commented Feb 6, 2024

mlyublena Feb 6, 2024

mlyublena Feb 6, 2024

pranjalssh commented Feb 14, 2024

jaystarshot commented Mar 4, 2024 •

edited

Loading

pranjalssh Mar 6, 2024

github-actions bot commented Mar 6, 2024 •

edited

Loading

jaystarshot commented Mar 6, 2024

	HEURISTIC, // Materialze CTES occuring > CTE_HEURISTIC_REPLICATION_THRESHOLD
	HEURISTIC, // Materialize CTES occuring > CTE_HEURISTIC_REPLICATION_THRESHOLD

Add Heuristic CTE Materialization Strategy #21720

Add Heuristic CTE Materialization Strategy #21720

Conversation

jaystarshot commented Jan 17, 2024 • edited Loading

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

jaystarshot commented Jan 31, 2024

mlyublena Jan 31, 2024

Choose a reason for hiding this comment

mlyublena Jan 31, 2024

Choose a reason for hiding this comment

mlyublena Jan 31, 2024

Choose a reason for hiding this comment

mlyublena commented Jan 31, 2024

jaystarshot commented Feb 2, 2024

jaystarshot commented Feb 6, 2024

mlyublena Feb 6, 2024

Choose a reason for hiding this comment

mlyublena Feb 6, 2024

Choose a reason for hiding this comment

pranjalssh commented Feb 14, 2024

jaystarshot commented Mar 4, 2024 • edited Loading

pranjalssh Mar 6, 2024

Choose a reason for hiding this comment

github-actions bot commented Mar 6, 2024 • edited Loading

jaystarshot commented Mar 6, 2024

jaystarshot commented Jan 17, 2024 •

edited

Loading

jaystarshot commented Mar 4, 2024 •

edited

Loading

github-actions bot commented Mar 6, 2024 •

edited

Loading