span: use KeyColumns in span.Builder#76836
Conversation
|
This change is |
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, 2 of 2 files at r2, 7 of 7 files at r3, 10 of 10 files at r4, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)
pkg/sql/rowexec/inverted_joiner.go, line 457 at r3 (raw file):
ij.indexRow[prefixIdx] = row[colIdx] } // TODO(mgartner): MakeKeyFromEncDatumsDeprecated will allocate and grow a
nit: I don't see MakeKeyFromEncDatumsDeprecated anywhere.
pkg/sql/span/span_builder.go, line 83 at r1 (raw file):
} // SpanFromEncDatums encodes a span with prefixLen constraint columns from the
nit: prefixLen is no more.
pkg/sql/span/span_builder.go, line 263 at r2 (raw file):
// We need to advance the end key if it is inclusive or the index is // inverted. if cs.EndBoundary() == constraint.IncludeBoundary || s.index.GetType() == descpb.IndexDescriptor_INVERTED {
This change seems like it doesn't belong to the third commit. It'd be good to extra it into a separate commit (maybe separate PR) with the corresponding reasoning, and probably get a sign-off on this from someone else.
pkg/sql/span/span_builder.go, line 98 at r3 (raw file):
} makeKeyFromRow := func(r rowenc.EncDatumRow) (k roachpb.Key, cn bool, e error) {
nit: this function seems to not be very useful anymore, maybe delete it entirely?
|
pkg/sql/span/span_builder.go, line 263 at r2 (raw file): Previously, yuzefovich (Yahor Yuzefovich) wrote…
I was going to ask you about this check, which seems wrong/unnecessary. I believe you added it in 55bb02f Do you remember why? |
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/span/span_builder.go, line 263 at r2 (raw file):
Previously, RaduBerinde wrote…
I was going to ask you about this check, which seems wrong/unnecessary. I believe you added it in 55bb02f Do you remember why?
The logic was already present in AdjustEndKeyForInterleave which was removed in that commit, so I only inlined what was relevant.
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/span/span_builder.go, line 263 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
The logic was already present in
AdjustEndKeyForInterleavewhich was removed in that commit, so I only inlined what was relevant.
Ah, I see. It looks like an early return path because inverted index can't be interleaved, but it failed to account for the inclusive flag. It looks like it has been there for a long time (since knz@21c2b80), before we could have exclusive boundaries for inverted indexes. I'll open up a separate PR to remove.
4d60154 to
61ac25d
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/sql/span/span_builder.go, line 263 at r2 (raw file):
Previously, RaduBerinde wrote…
Ah, I see. It looks like an early return path because inverted index can't be interleaved, but it failed to account for the
inclusiveflag. It looks like it has been there for a long time (since knz@21c2b80), before we could have exclusive boundaries for inverted indexes. I'll open up a separate PR to remove.
Will rebase on top of #76865 when that merges.
pkg/sql/span/span_builder.go, line 98 at r3 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: this function seems to not be very useful anymore, maybe delete it entirely?
It makes the four callsites less cluttered. I left it (but I did remove the unnamed return).
f1504a3 to
018dcec
Compare
Release note: None
EncodeIndexKey has a special NULL check for primary indexes. This check is a bit out of left field in this code, which is meant to be more generic. This commit moves this check in the higher-level primary index encoding routines and simplifies some related code. We no longer try to plumb the null ColumnID, instead we figure it out when generating the error. Release note: None
This commit refactors span.Builder to use `IndexFetchSpec_KeyColumn`s instead of descriptors. This is in preparation for replacing the JoinReader descriptor with an `IndexFetchSpec`. Release note: None
This commit replaces `MakeBuilder` (which allocates a `*Builder`) with an `Init` method. This should be more efficient and also simplifies the code by removing the pool. Release: None
018dcec to
cc17d2a
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
RFAL (the last commit is new)
Reviewable status:
pkg/sql/span/span_builder.go, line 263 at r2 (raw file):
Previously, RaduBerinde wrote…
Will rebase on top of #76865 when that merges.
Done.
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 16 of 16 files at r5, 2 of 2 files at r6, 7 of 7 files at r7, 11 of 11 files at r8, all commit messages.
Reviewable status:
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
span: minor span.Builder cleanup
Release note: None
rowenc: move null PK check
EncodeIndexKey has a special NULL check for primary indexes. This
check is a bit out of left field in this code, which is meant to be
more generic.
This commit moves this check in the higher-level primary index
encoding routines and simplifies some related code. We no longer try
to plumb the null ColumnID, instead we figure it out when generating
the error.
Release note: None
span: use KeyColumns in span.Builder
This commit refactors span.Builder to use
IndexFetchSpec_KeyColumns instead of descriptors. This is inpreparation for replacing the JoinReader descriptor with an
IndexFetchSpec.Release note: None
span: don't allocate span.Builder
This commit replaces
MakeBuilder(which allocates a*Builder) withan
Initmethod. This should be more efficient and also simplifiesthe code by removing the pool.
Release: None