-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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.
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
Fix failures for EXPLAIN ANALYZE with CTEs #23824
base: master
Are you sure you want to change the base?
Conversation
0e64d8c
to
dbe88ac
Compare
presto-main/src/main/java/com/facebook/presto/operator/ExplainAnalyzeOperator.java
Outdated
Show resolved
Hide resolved
presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestDistributedQueries.java
Outdated
Show resolved
Hide resolved
presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestDistributedQueries.java
Outdated
Show resolved
Hide resolved
e3a5e8f
to
b075136
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! A few more comments
presto-hive/src/test/java/com/facebook/presto/hive/TestHiveDistributedQueries.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/com/facebook/presto/hive/TestHiveDistributedQueries.java
Outdated
Show resolved
Hide resolved
private final JsonCodec<Map<PlanFragmentId, JsonPlanFragment>> planMapCodec; | ||
private final JsonCodec<JsonRenderedNode> codec; | ||
private final JsonCodec<Map<PlanFragmentId, JsonPlan>> deserializationCodec; | ||
private final JsonCodec<List<Map<PlanFragmentId, JsonPlan>>> deserializationCodec; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will change the top-level JSON form of the EXPLAIN (format JSON, type DISTRIBUTED)
output as well as EXPLAIN ANALYZE (format JSON)
.
@aaneja do you have any objections to this? We will probably need to update some internal tooling to parse properly if this change goes through.
presto-hive/src/test/java/com/facebook/presto/hive/TestHiveDistributedQueries.java
Outdated
Show resolved
Hide resolved
92a00e9
to
59bf4e8
Compare
Thanks for the release note entry! Nit suggestion to rephrase to follow the Order of changes in the Release Note Guidelines.
|
The EXPLAIN ANALYZE operator only supports one substage when returning the output. When outputting in TEXT format, modify it to loop through the substages and return the substage ID and its plan. For JSON format, return a list of plans. Resolves: prestodb#23798
I think the root cause here is not with a failure in rendering, but with how the
To me, this look like a bug with how the plan got fragmented. I need to dive deeper to confirm this |
Description
The EXPLAIN ANALYZE operator only supports one substage when returning the output.
When outputting in TEXT format, modify it to loop through the substages and return
the substage ID and its plan.
For JSON format, return a list of plans.
Motivation and Context
Multiple substages were not previously supported in the output
Resolves: #23798
Impact
Modifies the EXPLAIN ANALYZE output
New text output
Old text output
New JSON output
Old JSON output
Test Plan
Tested locally using the HiveQueryRunner