-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
opt: remember subquery ASTs for explain #29597
Conversation
With the optimizer, this field will show the original subquery which might have been modified by transforms. Release note: None
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.
Reviewed 5 of 5 files at r1, 14 of 14 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/sql/opt/memo/expr_view_format.go, line 395 at r2 (raw file):
case opt.SubqueryOp, opt.ExistsOp: private = nil
We have comments above to explain why private
is set to nil for some operators -- might be worth doing that here too.
pkg/sql/opt/norm/rules/decorrelate.opt, line 689 at r2 (raw file):
(Filters [ (ConstructAnyCondition $input $scalar $subqueryDef) ]) ) $subqueryDef
Is there any issue with reusing the same subqueryDef even though this is a different subquery?
Store the original `*tree.Subquery` in `Any`, `Exists`, `Subquery` and use it to populate the subquery during execbuild. This causes the original subquery to show up in `EXPLAIN`. Fixes cockroachdb#29350. Release note: None
31b310b
to
3eab8d4
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.
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/sql/opt/memo/expr_view_format.go, line 395 at r2 (raw file):
Previously, rytaft wrote…
We have comments above to explain why
private
is set to nil for some operators -- might be worth doing that here too.
Done.
pkg/sql/opt/norm/rules/decorrelate.opt, line 689 at r2 (raw file):
Previously, rytaft wrote…
Is there any issue with reusing the same subqueryDef even though this is a different subquery?
No, that's the point of OriginalExpr
. It's true that we copy Cmp
too but it's fine since it's not used by Exists
.
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.
Reviewed 5 of 5 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (and 2 stale)
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.
Reviewed 1 of 1 files at r3.
Reviewable status:complete! 1 of 0 LGTMs obtained (and 1 stale)
bors r+ |
29597: opt: remember subquery ASTs for explain r=RaduBerinde a=RaduBerinde #### sql: rename "sql" to "original sql" in subquery EXPLAIN With the optimizer, this field will show the original subquery which might have been modified by transforms. Release note: None #### opt: remember subquery ASTs for explain Store the original `*tree.Subquery` in `Any`, `Exists`, `Subquery` and use it to populate the subquery during execbuild. This causes the original subquery to show up in `EXPLAIN`. Fixes #29350. Release note: None Co-authored-by: Radu Berinde <[email protected]>
Build succeeded |
sql: rename "sql" to "original sql" in subquery EXPLAIN
With the optimizer, this field will show the original subquery which
might have been modified by transforms.
Release note: None
opt: remember subquery ASTs for explain
Store the original
*tree.Subquery
inAny
,Exists
,Subquery
anduse it to populate the subquery during execbuild. This causes the
original subquery to show up in
EXPLAIN
.Fixes #29350.
Release note: None