-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Plan queries with and without EXPLAIN ANALYZE the same way #26938
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
Conversation
Reviewer's GuideThis PR ensures that EXPLAIN ANALYZE queries are planned the same way as regular queries by normalizing the additional ExplainAnalyze wrapper and top-level exchanges, refactors AddExchanges to always introduce a gathering exchange with undistributed properties for ExplainAnalyzeNode, and strengthens coverage with new parameterized tests for TPCH and TPCDS workloads. Class diagram for AddExchanges changes (ExplainAnalyzeNode planning)classDiagram
class AddExchanges {
+visitExplainAnalyze(node, preferredProperties)
}
class ExplainAnalyzeNode
class ExchangeNode {
+Type: enum {GATHER, ...}
}
class PlanWithProperties {
+getNode()
}
AddExchanges --> ExplainAnalyzeNode
AddExchanges --> ExchangeNode
AddExchanges --> PlanWithProperties
ExplainAnalyzeNode <|-- ExchangeNode
PlanWithProperties --> ExchangeNode
AddExchanges : planChild(node, PreferredProperties.undistributed())
AddExchanges : Always add gathering ExchangeNode for ExplainAnalyzeNode
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- The indentation stripping in assertExplainAnalyzePlan relies on exactly eight spaces and two header lines; consider computing the common indent and header length dynamically to make the test resilient to formatting changes.
- The two TestExplainAnalyze*Plan classes share almost identical logic except for table sources—consider parameterizing or consolidating them to reduce duplication.
- Removing the special-case check for existing gathering exchanges in AddExchanges may introduce redundant exchanges for ExplainAnalyze; ensure this change doesn’t inadvertently produce extra exchange nodes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The indentation stripping in assertExplainAnalyzePlan relies on exactly eight spaces and two header lines; consider computing the common indent and header length dynamically to make the test resilient to formatting changes.
- The two TestExplainAnalyze*Plan classes share almost identical logic except for table sources—consider parameterizing or consolidating them to reduce duplication.
- Removing the special-case check for existing gathering exchanges in AddExchanges may introduce redundant exchanges for ExplainAnalyze; ensure this change doesn’t inadvertently produce extra exchange nodes.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
9e7a4b0
to
c2954b7
Compare
@kasiafi Could you update "Release notes" section? |
Description
The change makes the query under
ExplainAnalyzeNode
plan the same way as underOutputNode
.Example query that has different plan shape before this change:
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(X) Release notes are required, with the following suggested text:
Summary by Sourcery
Unify query planning for EXPLAIN ANALYZE and regular queries by using undistributed preferred properties and always adding a gathering exchange, and expand test coverage to validate plan consistency across TPC-H and TPC-DS suites.
Enhancements:
Tests: