rowenc: various improvements to IndexFetchSpec initialization #76795
Conversation
This commit removes a check that fails creation of an IndexFetchSpec if a key column is not public. The only case where this comes up AFAICT is when we hit a duplicate key error and try to fetch the row to get the values for the error message. In this case, even though the column is not public, we know that the row we care about exists in the index, or we wouldn't have hit the error. Release note: None
|
This change is |
RaduBerinde
changed the title
RaduBerinde
force-pushed
the
fetch-stuff-in-descs
branch
from
c064af2 to
2be05a8
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 4 of 4 files at r2, 6 of 6 files at r3, 4 of 4 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)
pkg/sql/catalog/descriptor.go, line 512 at r2 (raw file):
IndexStoredColumns(idx Index) []Column // FetchSpecKeyAndSuffixColumns returns information about the key and suffix
nit: s/FetchSpec/IndexFetchSpec/.
pkg/sql/catalog/tabledesc/column.go, line 382 at r2 (raw file):
invertedColumnID = idx.InvertedColumnID() } var compositeIDs catalog.TableColSet
nit: I think you can do idx.CollectCompositeColumnIDs for this.
pkg/sql/catalog/tabledesc/column.go, line 390 at r2 (raw file):
col := ic.all[i] if col == nil { // This is an invalid descriptor.
When can this occur? Maybe extend a comment?
pkg/sql/rowenc/index_fetch.go, line 35 at r2 (raw file):
fetchColumnIDs []descpb.ColumnID, ) error { //oldKeyAndSuffixCols := s.KeyAndSuffixColumns
nit: seems like there are some leftovers in this file in the second commit.
We now cache the IndexFetchSpec_KeyAndSuffixColumns in the table descriptor, making it cheaper to populate an IndexFetchSpec. Release note: None
This change renames KeysPerRow to IndexKeysPerRow and changes the argument to an IndexDescriptor, removing the error case. Release note: None
We now cache the `IndexFetchSpec_FamilyDefaultColumn`s in the table descriptor, to avoid an allocation when creating an `IndexFetchSpec`. Release note: None
We now calculate the key prefix length directly instead of creating a throw-away key. Release note: None
RaduBerinde
force-pushed
the
fetch-stuff-in-descs
branch
from
2be05a8 to
9515fc3
Compare
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/sql/catalog/tabledesc/column.go, line 382 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: I think you can do
idx.CollectCompositeColumnIDsfor this.
That's an Index method, we only have a descpb.IndexDescriptor here.
pkg/sql/catalog/tabledesc/column.go, line 390 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
When can this occur? Maybe extend a comment?
Added to a comment above. There are other weird things I had to tolerate (like nil columns or 0 column IDs).
|
bors r+ |
|
Build failed: |
There was a problem hiding this comment.
bors r+
Reviewable status:
|
Build succeeded: |
First commit is #76788.
catalog: store IndexFetchSpec key columns in table descriptor
We now cache the IndexFetchSpec_KeyAndSuffixColumns in the table
descriptor, making it cheaper to populate an IndexFetchSpec.
Release note: None
catalog: cleanup KeysPerRow
This change renames KeysPerRow to IndexKeysPerRow and changes the
argument to an IndexDescriptor, removing the error case.
Release note: None
catalog: cache family default columns in table descriptor
We now cache the
IndexFetchSpec_FamilyDefaultColumns in the tabledescriptor, to avoid an allocation when creating an
IndexFetchSpec.Release note: None
rowenc: calculate key prefix length without allocating
We now calculate the key prefix length directly instead of creating a
throw-away key.
Release note: None