-
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: support view and sequence numeric refs #39062
Conversation
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! 1 of 0 LGTMs obtained (waiting on @justinj, @RaduBerinde, and @rytaft)
pkg/sql/logictest/testdata/logic_test/numeric_references, line 13 at r1 (raw file):
statement error cannot specify an explicit column list when accessing a view by reference SELECT * FROM [54(1) AS _]
you can use let
to get the descriptor id and use that instead of hardcoding it (see https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/logictest/testdata/logic_test/prepare#L1030)
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 2 of 4 files at r1, 2 of 2 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @justinj)
pkg/sql/opt/testutils/testcat/test_catalog.go, line 159 at r2 (raw file):
return ds, nil } if v, ok := ds.(*View); ok && v.ViewID == id {
is it worth having an ID()
method in the DataSource
interface that all of these implement?
pkg/sql/opt/testutils/testcat/test_catalog.go, line 159 at r2 (raw file): Previously, rytaft (Rebecca Taft) wrote…
Actually, |
TFTRs! bors r+ |
Build failed |
I ended up complicating this much more than necessary - it turns out the heuristic planner doesn't even support specifying columns for view numeric references, so this feature is actually really straightforward. I added a separate test file that is run under the heuristic planner as well (the existing numeric reference test file panics when run under the HP, concerningly...). Release note: None
bors r+ |
Build failed (retrying...) |
39062: opt: support view and sequence numeric refs r=justinj a=justinj I ended up complicating this much more than necessary - it turns out the heuristic planner doesn't even support specifying columns for view numeric references, so this feature is actually really straightforward. I added a separate test file that is run under the heuristic planner as well (the existing numeric reference test file panics when run under the HP, concerningly...). Release note: None 39147: opt: add rowsort to with-opt tests r=justinj a=justinj Fixes #39112. Thanks for finding @tbg! Release note: None Co-authored-by: Justin Jaffray <[email protected]>
Build succeeded |
I ended up complicating this much more than necessary - it turns out the
heuristic planner doesn't even support specifying columns for view
numeric references, so this feature is actually really straightforward.
I added a separate test file that is run under the heuristic planner as
well (the existing numeric reference test file panics when run under the
HP, concerningly...).
Release note: None