-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
sql: fix UNIQUE column definition for REGIONAL BY ROW #61449
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.
Reviewed 2 of 2 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajstorm and @otan)
pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 1799 at r1 (raw file):
statement ok CREATE TABLE regional_by_row_unique_in_column_def (
[nit] I think you want to name this table ....abc instead of ....def
pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 1823 at r1 (raw file):
CREATE TABLE regional_by_row_fk_reference ( d INT PRIMARY KEY, e INT REFERENCES regional_by_row_unique_in_column_def(a),
How about also adding UNIQUE for one of these here? e.g.,
e INT UNIQUE REFERENCES regional_by_row_unique_in_column_def(a),
pkg/sql/create_table.go, line 1809 at r1 (raw file):
for _, implicitColumnDefIdx := range implicitColumnDefIdxs { // If it a non-primary index that is implicitly created, ensure partitioning
[nit] If it a -> If it is a
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 @ajstorm and @rytaft)
pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 1799 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] I think you want to name this table ....abc instead of ....def
Done.
pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 1823 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
How about also adding UNIQUE for one of these here? e.g.,
e INT UNIQUE REFERENCES regional_by_row_unique_in_column_def(a),
Done.
pkg/sql/create_table.go, line 1809 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] If it a -> If it is a
Done.
This was buggy as the implicitly added crdb_region column was not initialized before the UNIQUE definition was available. Instead, defer these index creations until after all column definitions have been initialized. Release justification: low-risk bug fix to new functionality 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 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajstorm)
pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 1799 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
Done.
Oh sorry -- I thought you were naming it after the columns and was confused about why you said "def" instead of "abc". Feel free to change it back to _def if you prefer (I see now that you were referring to the ColumnDef from the AST). Fine with me either way
pkg/ccl/logictestccl/testdata/logic_test/regional_by_row, line 1823 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
Done.
No need to make them both unique -- might be nice to have one of each (but again -- I don't have a strong opinion here)
hah it's fine, i'll live without the commit extra round trip :D bors r=rytaft |
Build succeeded: |
This was buggy as the implicitly added crdb_region column was not
initialized before the UNIQUE definition was available. Instead, defer
these index creations until after all column definitions have been
initialized.
Release justification: low-risk bug fix to new functionality
Release note: None