Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support format arguments in EXPLAIN ANALYZE #22733

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support format arguments in EXPLAIN ANALYZE #22733

wants to merge 1 commit into from

Conversation

ZacBlanco
Copy link
Contributor

@ZacBlanco ZacBlanco commented May 13, 2024 •
edited

Description

This change adds two features:

  1. Support for format <X> in EXPLAIN ANALYZE
  2. 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|GRAPHVIZ>`` :pr:`22733`

@ZacBlanco ZacBlanco force-pushed the upstream-stats-explain-analyze branch 6 times, most recently from b760bb9 to 78d8af9 Compare May 20, 2024 21:37
@ZacBlanco ZacBlanco force-pushed the upstream-stats-explain-analyze branch from 78d8af9 to 4774032 Compare May 21, 2024 23:56
@ZacBlanco ZacBlanco marked this pull request as ready for review May 22, 2024 19:40
@ZacBlanco ZacBlanco requested a review from presto-oss May 22, 2024 19:40
steveburnett
steveburnett previously approved these changes May 22, 2024
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (docs)

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

@steveburnett
Copy link
Contributor

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 aaneja self-requested a review May 28, 2024 16:16
aaneja
aaneja reviewed May 28, 2024
View reviewed changes
presto-main/src/main/java/com/facebook/presto/operator/ExplainAnalyzeOperator.java Outdated
Comment on lines 166 to 167
case GRAPHVIZ:
plan = graphvizDistributedPlan(queryInfo.getOutputStage().get().getSubStages().get(0), functionAndTypeManager, operatorContext.getSession());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ClarenceThreepwood
ClarenceThreepwood reviewed May 28, 2024
View reviewed changes
presto-main/src/main/java/com/facebook/presto/sql/planner/plan/ExplainAnalyzeNode.java Outdated Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/LogicalPlanner.java Outdated Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/operator/ExplainAnalyzeOperator.java Outdated
Comment on lines 166 to 167
case GRAPHVIZ:
plan = graphvizDistributedPlan(queryInfo.getOutputStage().get().getSubStages().get(0), functionAndTypeManager, operatorContext.getSession());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ZacBlanco ZacBlanco force-pushed the upstream-stats-explain-analyze branch from 4774032 to 6d1b2fd Compare May 28, 2024 19:01
@ZacBlanco ZacBlanco force-pushed the upstream-stats-explain-analyze branch from 6d1b2fd to ddf57b1 Compare May 29, 2024 14:58
@ZacBlanco
Add execution stats to JSON distributed plan
741acd4 
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 ZacBlanco force-pushed the upstream-stats-explain-analyze branch from ddf57b1 to 741acd4 Compare May 29, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaneja aaneja aaneja approved these changes

@steveburnett steveburnett steveburnett left review comments

@ClarenceThreepwood ClarenceThreepwood ClarenceThreepwood left review comments

@jaystarshot jaystarshot Awaiting requested review from jaystarshot jaystarshot is a code owner

@feilong-liu feilong-liu Awaiting requested review from feilong-liu feilong-liu is a code owner

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ZacBlanco @steveburnett @aaneja @ClarenceThreepwood