sql: cleanup handling of EXPLAIN in the new factory #52108
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
yuzefovich
force-pushed
the
distsql-cleanup
branch
from
c94d5d9
to
6f518de
Compare
yuzefovich
force-pushed
the
distsql-cleanup
branch
2 times, most recently
from
5413e15
to
9b64d3b
Compare
Release note: None
Previously, several EXPLAIN variants were mishandled in the new factory: instead of being wrapped as other planNodes are they were kept separate which resulted in a "mixed"-representation plan. This is now fixed by properly wrapping them. Additionally, this commit removes "partial support" that we had for EXPLAIN (PLAN) - the new factory cannot currently implement it, so in such cases we will be falling back to the old factory. Release note: None
yuzefovich
force-pushed
the
distsql-cleanup
branch
from
9b64d3b
to
08090b6
Compare
asubiotto
approved these changes
Aug 4, 2020
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, 11 of 11 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/distsql_physical_planner.go, line 449 at r1 (raw file):
// WITH ORDINALITY never gets distributed so that the gateway node can // always number each row in order. return cannotDistribute, nil
Does this fix a bug? If so, you should call that out in the commit message.
yuzefovich
commented
Aug 4, 2020
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/sql/distsql_physical_planner.go, line 449 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Does this fix a bug? If so, you should call that out in the commit message.
No, it doesn't because this case was handled by default case below. I made this change mostly for cosmetics - I think default is a catch-all for planNodes that don't have an equivalent processor, but ordinality node does have an equivalent and we choose (or we have to) not distribute it, so IMO it's nice to have an explicit case here that calls out that we don't distribute ordinality stage.
|
Build succeeded: |
craig bot
pushed a commit
that referenced
this pull request
Aug 4, 2020
51305: geo,geoindex: use bounding box for geography coverings that are bad r=sumeerbhola a=sumeerbhola This change uses a covererInterface implementation for geography that notices when a covering is using top-level cells of all faces and in that case uses the bounding box to compute the covering. Also changed the bounding box calculation for geography shapes to use only the points and not first construct s2.Regions. The latter causes marginally bad shapes to continue to have bad coverings since the bounding box also covers the whole earth. Release note: None 51882: roachpb: panic when comparing a Lease to a non-lease r=andreimatei a=andreimatei Release note: None 52146: sql: remove local execution of projectSetNode and implement ConstructProjectSet in the new factory r=yuzefovich a=yuzefovich Depends on #52108. **sql: remove local execution of projectSetNode** We have project set processor which is always planned for `projectSetNode`, so this commit removes the dead code of its local execution. Additionally, it removes some unused fields and cleans up cancellation check of the processor. Release note: None **sql: implement ConstructProjectSet in the new factory** Addresses: #47473. Release note: None 52320: kvserver: enable merges in kvnemesis r=aayushshah15 a=aayushshah15 We had merges disabled because of the bugs tracked in #44878, but those have since been fixed by #46085 and #50265. Release note: None Co-authored-by: sumeerbhola <sumeer@cockroachlabs.com> Co-authored-by: Andrei Matei <andrei@cockroachlabs.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: Aayush Shah <aayush.shah15@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
sql: cleanup ordinality code a bit
Release note: None
sql: cleanup handling of EXPLAIN in the new factory
Previously, several EXPLAIN variants were mishandled in the new factory:
instead of being wrapped as other planNodes are they were kept separate
which resulted in a "mixed"-representation plan. This is now fixed by
properly wrapping them.
Release note: None