Add log for canonicalized plans in HBO by feilong-liu · Pull Request #22306 · prestodb/presto

feilong-liu · 2024-03-22T20:03:22Z

Description

Log the stats equivalent query plan and the canonicalized plan from HBO.

Motivation and Context

This is to help debug plan hash of HBO.

In HBO, a query will get a snapshot of the plan which we call stats equivalent plan. This plan is then canonicalized to get a new canonicalized plan, which later gets hashed to get a string. This hash string is used as map key to store and retrieve history.

However, one problem observed is that, for the same query, it can have different hash string. This is hard to debug, as it cannot be deterministically reproduced. In this PR, I log the stats equivalent and canonicalized plan, so as to help compare and debug such cases.

Impact

Will help debug HBO hash generation.

Test Plan

Tested 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 log of stats equivalent plan and canonicalized plan for HBO. This feature is controlled by session property `log_query_plans_used_in_history_based_optimizer`

feilong-liu · 2024-03-22T21:53:00Z

presto-main/src/main/java/com/facebook/presto/cost/HistoryBasedPlanStatisticsCalculator.java

                    profileStartTime = System.nanoTime();
                }
-                planNodesWithHash.addAll(getPlanNodeHashes(plan, session, false).values());
+                planNodesWithHash.addAll(getPlanNodeHashes(plan, session, false, plan == root).values());


Record the stats equivalent plan and canonicalized plan only 1) when registering the plan, i.e. hash generation was first calculated as following calls will hit cache 2) only for root node, the serialized plan will cover the whole plan tree

feilong-liu · 2024-03-22T21:53:38Z

presto-main/src/main/java/com/facebook/presto/cost/HistoryBasedPlanStatisticsCalculator.java


        PlanNode plan = resolveGroupReferences(planNode, lookup);
-        Map<PlanCanonicalizationStrategy, PlanNodeWithHash> allHashes = getPlanNodeHashes(plan, session, true);
+        Map<PlanCanonicalizationStrategy, PlanNodeWithHash> allHashes = getPlanNodeHashes(plan, session, true, false);


Do not record plan when getting statistics

feilong-liu · 2024-03-22T23:10:03Z

presto-main/src/main/java/com/facebook/presto/cost/HistoryBasedPlanStatisticsCalculator.java

+        if (recordPlan) {
+            Optional<String> statsEquivalentPlanString = serializeStatsEquivalentPlan(statsEquivalentPlanNode);
+            if (statsEquivalentPlanString.isPresent()) {
+                historyBasedStatisticsCacheManager.setStatsEquivalentPlan(session.getQueryId(), statsEquivalentPlanString.get());


Record stats equivalent plan

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

steveburnett · 2024-03-25T18:27:23Z

"This feature is controlled by session property log_query_plans_used_in_history_based_optimizer.

Suggest adding doc for the session property. Would http://prestodb.io/docs/current/plugin/redis-hbo-provider.html or http://prestodb.io/docs/current/admin/properties.html#optimizer-properties be the correct place for it?

github-actions · 2024-03-25T20:54:57Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 8b8c9e4...89ed79c.

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

steveburnett

Thanks for the doc! One tiny nit suggestion for readability.

steveburnett · 2024-03-25T21:07:33Z

presto-docs/src/main/sphinx/admin/properties.rst

+    * **Type:** ``boolean``
+    * **Default value:** ``false``
+
+    Log stats equivalent plan and canonicalized plans used in history based optimization.


Suggested change

Log stats equivalent plan and canonicalized plans used in history based optimization.

Log the stats equivalent plan and canonicalized plans used in history based optimization.

"Log stats equivalent plan" seems a little hard to parse. Suggestion of "the" taken from the Description of the PR makes it easier to understand.

steveburnett

Thanks for the quick response! Two small nits.

presto-docs/src/main/sphinx/admin/properties.rst

steveburnett

LGTM! (docs)

Pull updated branch, new local build,looks good. Thanks for the rapid response!

pranjalssh · 2024-03-26T19:05:18Z

presto-main/src/main/java/com/facebook/presto/cost/HistoryBasedPlanStatisticsCalculator.java

 import static com.facebook.presto.SystemSessionProperties.getHistoryBasedOptimizerTimeoutLimit;
 import static com.facebook.presto.SystemSessionProperties.getHistoryInputTableStatisticsMatchingThreshold;
 import static com.facebook.presto.SystemSessionProperties.isVerboseRuntimeStatsEnabled;
+import static com.facebook.presto.SystemSessionProperties.logQueryPlansUsedInHBO;


Some places we use HBO, but some places we exapnd to historybasedoptimizer in session props/configs/func names. Lets keep it consistent

Changed to logQueryPlansUsedInHistoryBasedOptimizer

pranjalssh · 2024-03-26T19:05:53Z

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


    @Override
-    public Optional<String> hash(Session session, PlanNode planNode, PlanCanonicalizationStrategy strategy, boolean cacheOnly)
+    public Optional<String> hash(Session session, PlanNode planNode, PlanCanonicalizationStrategy strategy, boolean cacheOnly, boolean recordPlan)


Do we really need this boolean here. Putting it in hash api seems weird. Can we always record it, and choose to log it based on configs above this API.

Removed the record argument.

It will help to debug the root cause when the same query produces different plan hash.

pranjalssh

LGTM

feilong-liu requested review from a team, jaystarshot and shrinidhijoshi as code owners March 22, 2024 20:03

feilong-liu requested a review from presto-oss March 22, 2024 20:03

feilong-liu marked this pull request as draft March 22, 2024 20:03

feilong-liu force-pushed the log_hbo_canonical_plan branch 3 times, most recently from c7fae5f to efd39b0 Compare March 22, 2024 23:24

feilong-liu commented Mar 22, 2024

View reviewed changes

feilong-liu marked this pull request as ready for review March 22, 2024 23:27

jaystarshot reviewed Mar 23, 2024

View reviewed changes

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java Outdated Show resolved Hide resolved

feilong-liu requested review from mlyublena, pranjalssh and rschlussel March 25, 2024 16:45

feilong-liu force-pushed the log_hbo_canonical_plan branch from efd39b0 to 81ff8ce Compare March 25, 2024 18:16

feilong-liu requested a review from jaystarshot March 25, 2024 18:17

feilong-liu force-pushed the log_hbo_canonical_plan branch from 81ff8ce to 9c30696 Compare March 25, 2024 18:18

jaystarshot previously approved these changes Mar 25, 2024

View reviewed changes

feilong-liu dismissed jaystarshot’s stale review via b71ef34 March 25, 2024 20:54

feilong-liu force-pushed the log_hbo_canonical_plan branch from 9c30696 to b71ef34 Compare March 25, 2024 20:54

feilong-liu requested a review from steveburnett as a code owner March 25, 2024 20:54

steveburnett requested changes Mar 25, 2024

View reviewed changes

feilong-liu requested a review from steveburnett March 25, 2024 22:15

steveburnett requested changes Mar 25, 2024

View reviewed changes

presto-docs/src/main/sphinx/admin/properties.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/admin/properties.rst Outdated Show resolved Hide resolved

feilong-liu force-pushed the log_hbo_canonical_plan branch from b71ef34 to f7f89c4 Compare March 25, 2024 22:34

feilong-liu requested a review from steveburnett March 25, 2024 22:34

steveburnett previously approved these changes Mar 25, 2024

View reviewed changes

feilong-liu dismissed steveburnett’s stale review via 280e466 March 26, 2024 00:25

feilong-liu force-pushed the log_hbo_canonical_plan branch from f7f89c4 to 280e466 Compare March 26, 2024 00:25

jaystarshot previously approved these changes Mar 26, 2024

View reviewed changes

pranjalssh reviewed Mar 26, 2024

View reviewed changes

feilong-liu dismissed jaystarshot’s stale review via 3fa5d45 March 26, 2024 21:35

feilong-liu force-pushed the log_hbo_canonical_plan branch from 280e466 to 3fa5d45 Compare March 26, 2024 21:35

Add log for canonicalized plans in HBO

89ed79c

It will help to debug the root cause when the same query produces different plan hash.

feilong-liu force-pushed the log_hbo_canonical_plan branch from 3fa5d45 to 89ed79c Compare March 26, 2024 21:54

feilong-liu requested a review from pranjalssh March 26, 2024 22:11

pranjalssh approved these changes Mar 29, 2024

View reviewed changes

pranjalssh merged commit 2c226fc into prestodb:master Mar 29, 2024

feilong-liu deleted the log_hbo_canonical_plan branch March 29, 2024 05:57

feilong-liu restored the log_hbo_canonical_plan branch April 4, 2024 18:07

wanglinsong mentioned this pull request May 1, 2024

Add release notes for 0.287 #22647

Merged

48 tasks

	Log stats equivalent plan and canonicalized plans used in history based optimization.
	Log the stats equivalent plan and canonicalized plans used in history based optimization.

Conversation

feilong-liu commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steveburnett commented Mar 25, 2024

Uh oh!

github-actions bot commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranjalssh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feilong-liu commented Mar 22, 2024 •

edited

Loading

github-actions bot commented Mar 25, 2024 •

edited

Loading