span: remove INVERTED check from builder#76865
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Feb 22, 2022
Merged
Conversation
Member
|
This change is |
Member
Author
|
I didn't manage to find a query that results in an exclusive end span (in a real server or in the execbuilder tests). |
f960138 to
d6d32a5
Compare
yuzefovich
reviewed
Feb 21, 2022
Member
yuzefovich
left a comment
There was a problem hiding this comment.
I'll defer to Becca for approval.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @rytaft)
pkg/sql/span/span_builder.go, line 227 at r1 (raw file):
// roachpb.Spans and appends them to the provided spans. It appends multiple // spans in the case that multiple, non-adjacent column families should be // scanned. The forDelete parameter indicates whether these spans will be used
nit: forDelete has been removed recently.
pkg/sql/span/span_builder.go, line 264 at r1 (raw file):
} // We need to advance the end key if it is inclusive or the index is
nit: the comment needs an update.
This check has been carried over since we first added support for JSON indexes; at that point inverted spans were always single key and thus had inclusive boundaries. The check as it stands now is incorrect - there is no reason why we should always treat an end key as inclusive for an inverted index. In practice, the only current cases where we might have an exclusive end key are with geospatial queries, where the spans are not "tight" anyway (i.e. false positives are allowed). This might change in the future though, e.g. if we add support for range queries for JSON elements. Release note: None
d6d32a5 to
0ebada0
Compare
rytaft
approved these changes
Feb 22, 2022
Collaborator
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
RaduBerinde
commented
Feb 22, 2022
Contributor
|
Build succeeded: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This check has been carried over since we first added support for JSON
indexes; at that point inverted spans were always single key and thus
had inclusive boundaries.
The check as it stands now is incorrect - there is no reason why we
should always treat an end key as inclusive for an inverted index. In
practice, the only current cases where we might have an exclusive end
key are with geospatial queries, where the spans are not "tight"
anyway (i.e. false positives are allowed). This might change in the
future though, e.g. if we add support for range queries for JSON
elements.
Release note: None