sql: fix index column direction lookup bugs #58947
Conversation
|
This change is |
pkg/sql/catalog/tabledesc/index.go
Outdated
| @@ -273,6 +273,11 @@ func (w index) ForEachColumnID(fn func(colID descpb.ColumnID) error) error { | |||
| return w.desc.RunOverAllColumns(fn) | |||
| } | |||
|
|
|||
| // FirstExplicitColumnOrdinal returns the first ordinal of any explicit columns. | |||
| func (w index) FirstExplicitColumnOrdinal() int { | |||
| return w.desc.FirstExplicitColumnOrdinal() | |||
There was a problem hiding this comment.
This is in line with my recent changes from #58678 and the method renaming was done for the sake of naming consistency with the rest of catalog.Index.
There was a problem hiding this comment.
thanks for picking this up! have a few clarifying questions for my understanding
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @postamar)
pkg/sql/crdb_internal.go, line 2123 at r1 (raw file):
colName := tree.DNull colDir := tree.DNull if i >= len(index.IndexDesc().ColumnNames) {
nit: it seems like it would be a bit more readable if index.IndexDesc() were extracted into a separate variable (as line 2115 was doing before) any particular reason not to?
pkg/sql/pg_catalog.go, line 1453 at r1 (raw file):
indoption := tree.NewDArray(types.Int) colIDs := make([]descpb.ColumnID, 0, index.NumColumns()) for i := index.FirstExplicitColumnOrdinal(); i < index.NumColumns(); i++ {
actually i'm still confused what the original bug was. is the old ExplicitColumnStartIdx() not the same as the new FirstExplicitColumnOrdinal() ?
pkg/sql/pg_catalog.go, line 1496 at r1 (raw file):
tree.MakeDBool(tree.DBool(index.IsUnique())), // indimmediate tree.DBoolFalse, // indisclustered tree.MakeDBool(tree.DBool(index.Public())), // indisvalid
i'm not so familiar with this. why is index.Public() the same as !isMutation
pkg/sql/pg_catalog.go, line 1498 at r1 (raw file):
tree.MakeDBool(tree.DBool(index.Public())), // indisvalid tree.DBoolFalse, // indcheckxmin tree.MakeDBool(tree.DBool(index.WriteAndDeleteOnly())), // indisready
similar question, why is index.WriteAndDeleteOnly() equivalent?
pkg/sql/pg_catalog.go, line 1549 at r1 (raw file):
db *dbdesc.Immutable, table catalog.TableDescriptor, index catalog.Index,
should the function be renamed? it no longer seems to be "from a descriptor"
pkg/sql/catalog/tabledesc/index.go, line 278 at r1 (raw file):
Previously, postamar (Marius Posta) wrote…
This is in line with my recent changes from #58678 and the method renaming was done for the sake of naming consistency with the rest of
catalog.Index.
sounds good! a small request for the future: can you separate any renames/refactors into a commit separate from the commit that has functionality changes (and include both in the same PR). it helps with reviewing. definitely not a big deal for this PR, since the changes are small
This patch fixes a couple of instances in which we look up an index's column directions using a wrong ordinal when the index has implicit columns due to it being sharded or partitioned. Fixes cockroachdb#58945. Release note (bug fix): The indoption column in pg_catalog.index is now populated correctly.
There was a problem hiding this comment.
Thanks for the review. You're right there are too many unrelated changes in this PR. Here it is pared down to the bug fix, RFAL.
Reviewable status:
|
CI failed due to an unrelated test failure for which there is already an issue open: #58989 |
There was a problem hiding this comment.
thanks for the cleanup! i understand what's going on now :)
if you'd like to include your other refactoring, feel free to add that as another commit in this PR. if not, this lgtm!
Reviewable status:
|
i've re-run CI to see if it doesn't flake this time |
|
bors r+ Thanks, I think I'll open another PR for the rest of the changes, they're basically loose ends from #58678 , so I expect I'll find more of them. |
|
Build succeeded: |
This patch fixes a couple of instances in which we look up an index's
column directions using a wrong ordinal when the index has implicit
columns due to it being sharded or partitioned.
Fixes #58945.
Release note (bug fix): The indoption column in pg_catalog.index is now
populated correctly.