Add a new plan canonicalization strategy for history based optimizer #21832

feilong-liu · 2024-01-31T20:49:14Z

Description

Rename remove_safe_constants to ignore_safe_constants.

Add a new plan canonicalization strategy ignore_scan_constants for history based optimization. This is further relaxed compared with the currently used ignore_safe_constants.
For ignore_safe_constants, it will canonicalize constants in project and constants in predicates in scan node when the column is a partitioned column. For ignore_scan_constants, it will canonicalize constants in predicates in scan node, regardless of the column is a partitioned column or not.

Also add a session property history_optimization_plan_canonicalize_strategy to specify the canonicalization strategy to be used for HBO. It can be used to specify multiple strategies, and match the strategy with more strict criteria first.

Motivation and Context

If we know that a query is from a recurring workload, we can relax the match criteria so as to increase the coverage. Also we guarantee that the more relaxed strategy will match only when the other strategy does not match.

Impact

Increase coverage for HBO, with potential loss of accuracy as a tradeoff

Test Plan

Add unit tests, also test locally end to end

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 session property `history_optimization_plan_canonicalize_strategy` to specify the plan canonicalization strategies to use for HBO.
* Add a new plan canonicalization strategy `ignore_scan_constants` which canonicalize predicates for both partitioned and non-partitioned columns in scan node.

feilong-liu · 2024-02-07T19:26:16Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveTableLayoutHandle.java

@@ -290,7 +290,7 @@ private TupleDomain<ColumnHandle> getConstraint(PlanCanonicalizationStrategy can
                .transform(ColumnHandle.class::cast)
                .intersect(constraint);

-        constraint = constraint.canonicalize(HiveTableLayoutHandle::isPartitionKey);
+        constraint = canonicalizationStrategy.equals(PlanCanonicalizationStrategy.REMOVE_SCAN_CONSTANTS) ? constraint.canonicalize(x -> true) : constraint.canonicalize(HiveTableLayoutHandle::isPartitionKey);


If the strategy is REMOVE_SCAN_CONSTANTS, always canonicalize even when not a partitioned key

feilong-liu · 2024-02-07T19:30:17Z

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

+        List<PlanCanonicalizationStrategy> strategyList;
+        try {
+            strategyList = Splitter.on(",").trimResults().splitToList(session.getSystemProperty(HISTORY_OPTIMIZATION_PLAN_CANONICALIZE_STRATEGY, String.class)).stream()
+                    .map(x -> PlanCanonicalizationStrategy.valueOf(x)).sorted().collect(toImmutableList());


Call sorted here, so that the returned strategies are in order from more strict to less strict but more coverage.

This is a bit hard to manage.

I think we can remove CONNECTOR strategy(remove safe constants is pretty safe). We can then just use the session property to specify an integer(danger level) - which uses all strategies below that level. Is that sufficient?

I took a second look at it, I think it is easier to manage, but less flexible. For example, now we have ignore_safe_constant as danger level 1, and ignore_scan_constant as danger level 2, if people want to add a new strategy which is between ignore_safe_constant and ignore_scan_constant, we may need to re-assign the level of ignore_scan_constant to 3. This can be problematic as users are now setting the danger level in session property, and it will change behavior of existing settings.
Also it does not allow user to specify the specific strategies to use, but only able to specify a range of strategies.

In order to make it more explicit about the order of the strategies, I added a new field errorLevel in the PlanCanonicalizationStrategy enum.

feilong-liu · 2024-02-07T19:32:36Z

presto-spi/src/main/java/com/facebook/presto/spi/plan/PlanNodeWithHash.java

@@ -24,11 +26,13 @@ public class PlanNodeWithHash
    private final PlanNode planNode;
    // An optional canonical hash of the corresponding plan node
    private final Optional<String> hash;
+    private final PlanCanonicalizationStrategy planCanonicalizationStrategy;


Add strategy to hash, since different hash strategies result with different hash

CanonicalPlan already contains strategy - so hash contains the strategy. So we don't need it here

I see that the Map used in HistoryBasedPlanStatisticsProvider is using PlanNodeWithHash as key, should we keep the strategy? https://github.com/prestodb/presto/blob/master/presto-spi/src/main/java/com/facebook/presto/spi/statistics/HistoryBasedPlanStatisticsProvider.java#L37

Discussed offline, this field is not needed and removed it, also add comment that hash is already taking the strategy into account.

mlyublena · 2024-02-09T21:55:22Z

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

@@ -1528,6 +1532,14 @@ public SystemSessionProperties(
                        "When the size difference between current table and history table exceed this threshold, do not match history statistics",
                        0.0,
                        true),
+                stringProperty(
+                        HISTORY_OPTIMIZATION_PLAN_CANONICALIZE_STRATEGY,


should we just call this history_optimization_strategy? the current name is a bit long and has many internal details (that we canonicalize plans) + incorrect grammar

rename it to history_based_optimization_plan_canonicalization_strategy. I think it's OK to have details on the name, after all people who use it should know what they are doing.

mlyublena · 2024-02-09T21:56:24Z

presto-common/src/main/java/com/facebook/presto/common/plan/PlanCanonicalizationStrategy.java

@@ -61,15 +56,24 @@ public enum PlanCanonicalizationStrategy
     *
     * This is used in context of history based optimizations.
     */
-    REMOVE_SAFE_CONSTANTS;
+    REMOVE_SAFE_CONSTANTS,


perhaps a better name is ignore_safe_constants/ignore_scan_constants

Agree this is better. Just this will include more renaming changes in the PR, which I want to avoid to keep this PR simple.

since this is a user-facing parameter value, we might not be able to rename it later when it's already in use

Good point, renamed them.

steveburnett · 2024-02-09T22:14:43Z

Please consider documenting any new session properties.

pranjalssh · 2024-02-14T19:14:10Z

presto-common/src/main/java/com/facebook/presto/common/plan/PlanCanonicalizationStrategy.java

@@ -48,7 +43,7 @@ public enum PlanCanonicalizationStrategy
     */
    CONNECTOR,
    /**
-     * REMOVE_SAFE_CONSTANTS strategy is used to canonicalize plan with
+     * IGNORE_SAFE_CONSTANTS strategy is used to canonicalize plan with


pranjalssh · 2024-02-14T19:18:39Z

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

+        List<PlanCanonicalizationStrategy> strategyList;
+        try {
+            strategyList = Splitter.on(",").trimResults().splitToList(session.getSystemProperty(HISTORY_OPTIMIZATION_PLAN_CANONICALIZE_STRATEGY, String.class)).stream()
+                    .map(x -> PlanCanonicalizationStrategy.valueOf(x)).sorted().collect(toImmutableList());


This is a bit hard to manage.

I think we can remove CONNECTOR strategy(remove safe constants is pretty safe). We can then just use the session property to specify an integer(danger level) - which uses all strategies below that level. Is that sufficient?

feilong-liu · 2024-02-15T19:24:46Z

Please consider documenting any new session properties.

Added

github-actions · 2024-02-15T19:25:21Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 56a0910...de56a33.

Notify	File(s)
@steveburnett	presto-docs/src/main/sphinx/admin/properties.rst

steveburnett

LGTM! (docs)

steveburnett · 2024-05-03T17:41:13Z

@feilong-liu, reviewing this while working on #22647, I noticed that the new in this PR session properties ignore_scan_constants and history_optimization_plan_canonicalize_strategy were not documented in https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/admin/properties.rst.

Would you be willing to open a new PR and add documentation for these two properties?

feilong-liu requested review from shrinidhijoshi and a team as code owners January 31, 2024 20:49

feilong-liu requested a review from presto-oss January 31, 2024 20:49

feilong-liu marked this pull request as draft January 31, 2024 20:49

feilong-liu force-pushed the hbo_strategy branch 5 times, most recently from 6cfd80a to a78c497 Compare February 7, 2024 19:34

feilong-liu force-pushed the hbo_strategy branch from a78c497 to 33e71ee Compare February 9, 2024 18:31

feilong-liu commented Feb 9, 2024

View reviewed changes

feilong-liu marked this pull request as ready for review February 9, 2024 18:35

feilong-liu requested review from pranjalssh, jaystarshot and mlyublena February 9, 2024 18:35

feilong-liu force-pushed the hbo_strategy branch 2 times, most recently from e126050 to 76cb625 Compare February 9, 2024 21:08

mlyublena reviewed Feb 9, 2024

View reviewed changes

feilong-liu force-pushed the hbo_strategy branch 4 times, most recently from a84f19c to 5d8e28d Compare February 13, 2024 07:13

feilong-liu requested a review from mlyublena February 13, 2024 20:42

pranjalssh reviewed Feb 14, 2024

View reviewed changes

feilong-liu force-pushed the hbo_strategy branch from 5d8e28d to f6419b1 Compare February 15, 2024 06:07

feilong-liu requested a review from pranjalssh February 15, 2024 17:54

mlyublena approved these changes Feb 15, 2024

View reviewed changes

feilong-liu force-pushed the hbo_strategy branch from f6419b1 to c96ca28 Compare February 15, 2024 19:00

feilong-liu force-pushed the hbo_strategy branch from c96ca28 to f4e9eee Compare February 15, 2024 19:03

Add a new plan canonicalization strategy for history based optimizer

de56a33

feilong-liu force-pushed the hbo_strategy branch from f4e9eee to de56a33 Compare February 15, 2024 19:24

pranjalssh approved these changes Feb 15, 2024

View reviewed changes

feilong-liu merged commit 32628cd into prestodb:master Feb 15, 2024
57 checks passed

feilong-liu deleted the hbo_strategy branch February 15, 2024 20:37

steveburnett reviewed Feb 15, 2024

View reviewed changes

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 a new plan canonicalization strategy for history based optimizer #21832

Add a new plan canonicalization strategy for history based optimizer #21832

feilong-liu commented Jan 31, 2024 •

edited

Loading

feilong-liu Feb 7, 2024

feilong-liu Feb 7, 2024

pranjalssh Feb 14, 2024

feilong-liu Feb 14, 2024

feilong-liu Feb 7, 2024

pranjalssh Feb 14, 2024 •

edited

Loading

feilong-liu Feb 15, 2024

feilong-liu Feb 15, 2024

mlyublena Feb 9, 2024

feilong-liu Feb 12, 2024

mlyublena Feb 9, 2024

feilong-liu Feb 12, 2024

mlyublena Feb 12, 2024

feilong-liu Feb 13, 2024

steveburnett commented Feb 9, 2024

pranjalssh Feb 14, 2024

pranjalssh Feb 14, 2024

feilong-liu commented Feb 15, 2024

github-actions bot commented Feb 15, 2024

steveburnett left a comment

steveburnett commented May 3, 2024

Add a new plan canonicalization strategy for history based optimizer #21832

Add a new plan canonicalization strategy for history based optimizer #21832

Conversation

feilong-liu commented Jan 31, 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

pranjalssh Feb 14, 2024 • 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

Choose a reason for hiding this comment

steveburnett commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feilong-liu commented Feb 15, 2024

github-actions bot commented Feb 15, 2024

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett commented May 3, 2024

feilong-liu commented Jan 31, 2024 •

edited

Loading

pranjalssh Feb 14, 2024 •

edited

Loading