Support format arguments in EXPLAIN ANALYZE by ZacBlanco · Pull Request #22733 · prestodb/presto

ZacBlanco · 2024-05-13T21:41:59Z

Description

This change adds two features:

Support for format <X> in EXPLAIN ANALYZE
Adds support for a stats field in each plan node representation

This allows the user to get a computer-friendly representation of the plan with the statistics. This can be useful for visualization or as input to other systems for plan node-level performance analysis.

Motivation and Context

Additional execution analysis capabilities through query plan JSON

Impact

The JSON representation of a plan from now includes a "stats" field representation statistics at runtime. Its contents map to PlanNodeStats.java
- This effects the plans sent from the event listener framework
- Backwards compatibility should be preserved as the change only introduces a new field in the JSON representation without modifying previously-present fields.

Test Plan

New tests in the parser and statement analyzer to verify user-supplied arguments.

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

== RELEASE NOTES ==

General Changes
* Improve :doc:`/sql/explain-analyze` statement to support a ``format`` argument with values of ``<TEXT|JSON>`` :pr:`22733`

steveburnett

LGTM! (docs)

Pull branch, local docs build, looks good. Thanks!

steveburnett · 2024-05-22T21:29:02Z

Suggest revising the release note entry following the Order of changes in the Release Notes Guidelines, also add a link to the existing documentation (I tested this link in a local build):

== RELEASE NOTES ==

General Changes
* Improve :doc:`/sql/explain-analyze` statement to support a ``format`` argument with values of ``<TEXT|JSON|GRAPHVIZ>`` :pr:`22733`

aaneja · 2024-05-28T17:24:52Z

presto-main/src/main/java/com/facebook/presto/operator/ExplainAnalyzeOperator.java

+            case GRAPHVIZ:
+                plan = graphvizDistributedPlan(queryInfo.getOutputStage().get().getSubStages().get(0), functionAndTypeManager, operatorContext.getSession());


Since we are not changing the Graphviz output, should we skip supporting EXPLAIN ANALYZE (FORMAT GRAPHIZ) ? (the output is same as EXPLAIN (FORMAT GRAPHIZ))
IMO, it will only add to confusion for users

Thanks for your opinion on this one. I had a similar thought but decided to just include it anyways. However, if you think it is confusing I will remove support for it

Yes, lets skip it for now. We can enhance Graphviz output in a different PR (if there's interest) and add it back

naive question - why is it hard to add execution stats to graphviz?

I don't think it's particularly difficult, but I'm not familiar with the graphviz format. It would just require some more time+effort to make it work properly

presto-main/src/main/java/com/facebook/presto/sql/planner/plan/ExplainAnalyzeNode.java

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

vivek-bharathan · 2024-05-28T18:22:45Z

presto-main/src/main/java/com/facebook/presto/operator/ExplainAnalyzeOperator.java

+            case GRAPHVIZ:
+                plan = graphvizDistributedPlan(queryInfo.getOutputStage().get().getSubStages().get(0), functionAndTypeManager, operatorContext.getSession());


naive question - why is it hard to add execution stats to graphviz?

ZacBlanco · 2024-06-03T16:24:38Z

@feilong-liu @jaystarshot would you like to take a look at this?

feilong-liu · 2024-06-03T17:36:26Z

@feilong-liu @jaystarshot would you like to take a look at this?

Will take a look and reply later today

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

rschlussel · 2024-06-07T14:47:08Z

presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestAnalyzer.java

+    @Test
+    public void testExplainAnalyzeFormatJson()
+    {
+        analyze("EXPLAIN ANALYZE (format JSON) SELECT * FROM t1");


these just check that we don't fail. is there a way to test/see what the new output looks like?

I added a deserialize method in the JsonRenderer class and an associated test inside of AbstractTestDistributedQueries

rschlussel · 2024-06-07T14:49:12Z

presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanNodeStats.java


    private final Duration planNodeScheduledTime;
    private final Duration planNodeCpuTime;
+    private final Duration planNodeBlockedWallTime;


why are these added here?

The main driver behind this change is that we have an external tool that helps visualize some plans from the JSON output of the event listener. The event listener output is the actually the same output as what you get from an EXPLAIN ANALYZE. However, you only get execution statistics in the text output from the event listener, but not the JSON one. So, to augment the event listener's JSON plan output to be closer to the text version, I added some more statistics to this class.

This change adds two features: 1. Support for `format JSON` in EXPLAIN ANALYZE 2. Adds supoprt for a `stats` field in each plan node representation This allows the user to get a computer-friendly representation of the plan with the statistics. This can be useful for visualization

ZacBlanco force-pushed the upstream-stats-explain-analyze branch 6 times, most recently from b760bb9 to 78d8af9 Compare May 20, 2024 21:37

ZacBlanco force-pushed the upstream-stats-explain-analyze branch from 78d8af9 to 4774032 Compare May 21, 2024 23:56

ZacBlanco marked this pull request as ready for review May 22, 2024 19:40

ZacBlanco requested review from a team, feilong-liu, jaystarshot and steveburnett as code owners May 22, 2024 19:40

ZacBlanco requested a review from presto-oss May 22, 2024 19:40

steveburnett previously approved these changes May 22, 2024

View reviewed changes

vivek-bharathan self-requested a review May 23, 2024 17:09

aaneja self-requested a review May 28, 2024 16:16

aaneja reviewed May 28, 2024

View reviewed changes

vivek-bharathan reviewed May 28, 2024

View reviewed changes

ZacBlanco dismissed steveburnett’s stale review via 6d1b2fd May 28, 2024 19:01

ZacBlanco force-pushed the upstream-stats-explain-analyze branch from 4774032 to 6d1b2fd Compare May 28, 2024 19:01

vivek-bharathan previously approved these changes May 28, 2024

View reviewed changes

ZacBlanco dismissed vivek-bharathan’s stale review via ddf57b1 May 29, 2024 14:58

ZacBlanco force-pushed the upstream-stats-explain-analyze branch 2 times, most recently from ddf57b1 to 741acd4 Compare May 29, 2024 15:55

aaneja previously approved these changes May 29, 2024

View reviewed changes

jaystarshot previously approved these changes Jun 3, 2024

View reviewed changes

ZacBlanco dismissed jaystarshot’s stale review via 382afa2 June 6, 2024 15:30

ZacBlanco dismissed aaneja’s stale review via 382afa2 June 6, 2024 15:30

ZacBlanco force-pushed the upstream-stats-explain-analyze branch from 741acd4 to 382afa2 Compare June 6, 2024 15:30

rschlussel reviewed Jun 7, 2024

View reviewed changes

ZacBlanco force-pushed the upstream-stats-explain-analyze branch from 382afa2 to 72a7570 Compare June 7, 2024 16:59

rschlussel approved these changes Jun 7, 2024

View reviewed changes

ZacBlanco merged commit f661a1c into prestodb:master Jun 7, 2024

wanglinsong mentioned this pull request Jun 25, 2024

Add release notes for 0.288 #23079

Merged

36 tasks

		case GRAPHVIZ:
		plan = graphvizDistributedPlan(queryInfo.getOutputStage().get().getSubStages().get(0), functionAndTypeManager, operatorContext.getSession());

Conversation

ZacBlanco commented May 13, 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!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

steveburnett commented May 22, 2024

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZacBlanco commented Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feilong-liu commented Jun 3, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZacBlanco Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ZacBlanco commented May 13, 2024 •

edited

Loading

ZacBlanco commented Jun 3, 2024 •

edited

Loading

ZacBlanco Jun 7, 2024 •

edited

Loading