-
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
Modified EXPLAIN ANALYZE output #23824
base: master
Are you sure you want to change the base?
Conversation
0e64d8c
to
dbe88ac
Compare
break; | ||
case JSON: | ||
plan = jsonDistributedPlan(queryInfo.getOutputStage().get().getSubStages().get(0), functionAndTypeManager, operatorContext.getSession()); | ||
StringJoiner planStringJoiner = new StringJoiner(",", "[", "]"); |
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.
Since the output needs to be valid JSON, we should probably use a JSON codec for lists and render it through that instead. It likely will require some more refactoring of the PlanPrinter
class to achieve that. Joining strings like this is prone to errors/generating invalid JSON
fragmentsList = renderer.deserialize((String) computeActual("EXPLAIN ANALYZE (format JSON) SELECT rank() OVER (PARTITION BY orderkey ORDER BY clerk DESC) FROM orders WHERE orderkey < 0").getOnlyValue()); | ||
for (Map<PlanFragmentId, JsonRenderer.JsonPlan> fragments : fragmentsList) { | ||
fragments.values().forEach(planFragment -> assertJsonNodesHaveStats(planFragment.getPlan())); | ||
} |
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.
we should add another test where we enable CTEs and verify that all stages exist in the plan output
for (Map<PlanFragmentId, JsonRenderer.JsonPlan> fragments : fragmentsList) { | ||
fragments.values().forEach(planFragment -> assertJsonNodesHaveStats(planFragment.getPlan())); | ||
} |
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.
nit: since you're using the stream API within the loop, you might as well do just use it for the whole thing:
for (Map<PlanFragmentId, JsonRenderer.JsonPlan> fragments : fragmentsList) { | |
fragments.values().forEach(planFragment -> assertJsonNodesHaveStats(planFragment.getPlan())); | |
} | |
fragmentsList.stream().map(fragments -> { | |
fragments.values().forEach(planFragment -> assertJsonNodesHaveStats(planFragment.getPlan())); | |
}); |
same for the other blocks in this test
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
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