sql: use IndexFetchSpec for inverted joiner#77875
sql: use IndexFetchSpec for inverted joiner#77875craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
|
This change is |
yuzefovich
left a comment
There was a problem hiding this comment.
Nice cleanup!
Reviewed 2 of 2 files at r1, 17 of 17 files at r2, 18 of 18 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @RaduBerinde)
-- commits, line 18 at r2:
nit: "change changes" sounds not good :)
pkg/sql/opt/cat/index.go, line 190 at r2 (raw file):
ImplicitPartitioningColumnCount() int // GeoConfig returns a geospatial index configuration. If non-nil, it
nit: "not-nil" is no longer applicable.
pkg/sql/rowexec/inverted_joiner.go, line 459 at r1 (raw file):
} else { keyCols := ij.desc.IndexFetchSpecKeyAndSuffixColumns(ij.index) // Encode the prefix key; we reuse a buffer to avoid extra allocations when appending values.
nit: seems like a long line.
This commit addresses a TODO in the inverted joiner. We now reuse a buffer for encoding the prefix key. In the general case we still need to make a copy since we are storing multiple prefix keys; but now we try to reuse the last copy if the prefix is the same. We also remove the temporary use of `indexRow` which is confusing here (it wasn't obvious at first but this code has nothing to do with index rows and is just using `indexRow` as a temporary buffer). Release note: None
This commit replaces `IsEmptyConfig`, `IsGeographyConfig`, `IsGeometryConfig` with methods on the `geoindex.Config` type. We also switch to passing configs by value which is cleaner and avoids some allocations. Release note: None
This commit reworks the InvertedJoiner to use an IndexFetchSpec instead of table and index descriptors. The "internal schema" now matches the fetched columns (leading to simplifications in both planning and execution code). Release note: None
7ade8f3 to
f43648a
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status:
|
bors r+ |
mgartner
left a comment
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 34 of 34 files at r4, 17 of 17 files at r5, 18 of 18 files at r6, all commit messages.
Reviewable status:
pkg/geo/geoindex/config.go, line 14 at r5 (raw file):
// IsEmpty returns whether the config contains a geospatial index // configuration.
nit: I know you just copied the comments from before, but "returns whether ..." is less clear than "returns true if ...". Ditto for the other comments below.
|
Build succeeded: |
rowexec: address TODO in inverted joiner
This commit addresses a TODO in the inverted joiner. We now reuse a
buffer for encoding the prefix key. In the general case we still need
to make a copy since we are storing multiple prefix keys; but now we
try to reuse the last copy if the prefix is the same.
We also remove the temporary use of
indexRowwhich is confusing here(it wasn't obvious at first but this code has nothing to do with index
rows and is just using
indexRowas a temporary buffer).Release note: None
geoindex: minor cleanup around Config
This change changes
IsEmptyConfig,IsGeographyConfig,IsGeometryConfigto methods on thegeoindex.Configtype.We also switch to passing configs by value which is cleaner and avoids
some allocations.
Release note: None
sql: use IndexFetchSpec for inverted joiner
This commit reworks the InvertedJoiner to use an IndexFetchSpec
instead of table and index descriptors.
The "internal schema" now matches the fetched columns (leading to
simplifications in both planning and execution code).
Release note: None