fetchpb: extract index fetch spec into a separate package #94116
Conversation
|
This change is |
yuzefovich
force-pushed
the
index-fetch-spec
branch
from
f7cbef6 to
4d2d5f0
Compare
There was a problem hiding this comment.
I'm extremely supportive of this extraction. One note is that @Xiang-Gu has been contemplating just today pulling some proto enums out into an enumpb or catenumpb package that is very low dependency. That'd be a good place for this IndexColumn_ASC/DESC symbol.
| import "sql/types/types.proto"; | ||
| import "sql/catalog/catpb/index.proto"; | ||
| import "geo/geoindex/config.proto"; | ||
|
|
||
| // IndexFetchSpec contains the subset of information (from TableDescriptor and | ||
| // IndexDescriptor) that is necessary to decode KVs into SQL keys and values. | ||
| message IndexFetchSpec { | ||
| message Column { | ||
| optional uint32 column_id = 1 [(gogoproto.nullable) = false, |
There was a problem hiding this comment.
Keep the cast type and drop it to (gogoproto.casttype) = "[github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ColumnID](http://github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ColumnID)"
yuzefovich
force-pushed
the
index-fetch-spec
branch
3 times, most recently
from
e5e4ab8 to
d8efbbb
Compare
There was a problem hiding this comment.
Thanks for the feedback! I extracted out catenumpb with a few things in #94160, and I think that commit is beneficial on its own, so we should merge it regardless of whether the index fetch spec will be used in the BatchRequest. I'll keep this PR about moving the index fetch spec. I'll try the spec cache idea before marking this as ready for review.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)
pkg/sql/catalog/fetchpb/index_fetch.proto line 25 at r1 (raw file):
Previously, ajwerner wrote…
Keep the cast type and drop it to
(gogoproto.casttype) = "[github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ColumnID](http://github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ColumnID)"
Good point, done.
yuzefovich
force-pushed
the
index-fetch-spec
branch
2 times, most recently
from
aae7195 to
6f71c69
Compare
yuzefovich
requested review from
jayshrivastava and
rharding6373
and removed request for
a team
There was a problem hiding this comment.
nit: I think it'd be fine or even good to put this not under sql/catalog. At this point, it's feeling like the big thing this is doing is saying that rows and column encoding is a shared concept between sql and KV. Maybe put it near rowenc? I feel like with minimal effort we could make keyside and valueside not depend on sql/catalog and then pull them out of the sql tree altogether?
Anyways, this 
Reviewed 4 of 119 files at r1, 8 of 103 files at r2, all commit messages.
Reviewable status:
This will allow us to import the index fetch spec into `roachpb` which is needed for the KV projection pushdown work. This required changing the custom cast types used in the spec from type aliases defined in `descpb` to one level lower ones from `catid`. Epic: None Release note: None
yuzefovich
force-pushed
the
index-fetch-spec
branch
from
6f71c69 to
3555c05
Compare
|
Your reasoning sounds good to me, so I left a TODO with that spirit (since I want to not get distracted for now) and will merge as is. TFTR! bors r+ |
|
Build succeeded: |
This will allow us to import the index fetch spec into
roachpbwhichis needed for the KV projection pushdown work.
This required changing the custom cast types used in the spec from type
aliases defined in
descpbto one level lower ones fromcatid.Epic: CRDB-14837
Release note: None