rowexec: fix inverted join with prefix columns with DESC direction #154914
Conversation
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
Member
|
This change is |
yuzefovich
force-pushed
the
fix-ij
branch
2 times, most recently
from
ff28a0a to
e8f5377
Compare
mgartner
approved these changes
Oct 7, 2025
Collaborator
mgartner
left a comment
mgartner
left a comment
There was a problem hiding this comment.
SpansFromInvertedSpans could be split into two methods: one for constraints and one for when the prefix has already been encoded in the inverted spans.
@mgartner reviewed 1 of 7 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained
mgartner
added
the
backport-all
This commit fixes a bug that was added long time ago when we added support for inverted joins on multi-column inverted indexes (i.e. such indexes that have a prefix of "forward" columns that we can constrain using an equality filter) in d3f9a8b. In particular, in that change we added tricky logic which prepended the prefix to the encoded inverted key; however, we then didn't change how we use the `span.Builder`. That guy assumes that it receives datums for each index column (both prefix and inverted) whereas we only provided the inverted bytes (with the prepended prefix). This led to us being confused about which key column is being encoded during span construction. Consider an example where we have one prefix key column like `i INT DESC` (followed by the inverted column). In this case, when calling `MakeKeyFromEncDatums` we'd try to encode the single EncDatum using the DESC key direction. The EncDatum internally stores the prefix + inverted key using ASC key direction, so we'd try to decode it and fail - because of the prepended prefix. After this patch, we'll correctly only use a single column in the call to `MakeKeyFromEncDatums`. Note that if prefix columns had only ASC directions, then we would simply get lucky. Namely, since we have EncDatumRow with a single EncDatum AND that EncDatum already has encoded value of ASC direction stored, we'd just reuse it and exit. The code here is quite difficult to follow and perhaps it could be refactored (e.g. the comment on `SpansFromInvertedSpans` says that multi-column inverted indexes are expected to use non-nil constraint, yet in the inverted joiner we always pass nil), but I chose a more targetted fix that seems reasonable. Additionally, to simplify the change a bit, I inlined the code for `rowenc.MakeSpanFromEncDatums` into two call sites (which also allows us to avoid constructing the unnecessary end key for the inverted spans case). Release note (bug fix): Previously, CockroachDB would hit an internal error when performing an inverted join using an inverted index in which the first prefix column had DESC direction. The bug has been present since 21.1 and is now fixed.
Member
Author
|
I agree about the refactor, left a TODO for that idea. TFTR! bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Oct 7, 2025
154781: sql: display notice with job ID when waiting for a job r=rafiss a=rafiss This will allow users to easily see the job ID in case they'd like to look at more details about the job in other introspection interfaces. related to: #104597 Epic: None Release note: None 154914: rowexec: fix inverted join with prefix columns with DESC direction r=yuzefovich a=yuzefovich This commit fixes a bug that was added long time ago when we added support for inverted joins on multi-column inverted indexes (i.e. such indexes that have a prefix of "forward" columns that we can constrain using an equality filter) in d3f9a8b. In particular, in that change we added tricky logic which prepended the prefix to the encoded inverted key; however, we then didn't change how we use the `span.Builder`. That guy assumes that it receives datums for each index column (both prefix and inverted) whereas we only provided the inverted bytes (with the prepended prefix). This led to us being confused about which key column is being encoded during span construction. Consider an example where we have one prefix key column like `i INT DESC` (followed by the inverted column). In this case, when calling `MakeKeyFromEncDatums` we'd try to encode the single EncDatum using the DESC key direction. The EncDatum internally stores the prefix + inverted key using ASC key direction, so we'd try to decode it and fail - because of the prepended prefix. After this patch, we'll correctly only use a single column in the call to `MakeKeyFromEncDatums`. Note that if prefix columns had only ASC directions, then we would simply get lucky. Namely, since we have EncDatumRow with a single EncDatum AND that EncDatum already has encoded value of ASC direction stored, we'd just reuse it and exit. The code here is quite difficult to follow and perhaps it could be refactored (e.g. the comment on `SpansFromInvertedSpans` says that multi-column inverted indexes are expected to use non-nil constraint, yet in the inverted joiner we always pass nil), but I chose a more targetted fix that seems reasonable. Additionally, to simplify the change a bit, I inlined the code for `rowenc.MakeSpanFromEncDatums` into two call sites (which also allows us to avoid constructing the unnecessary end key for the inverted spans case). Fixes: #153499. Release note (bug fix): Previously, CockroachDB would hit an internal error when performing an inverted join using an inverted index in which the first prefix column had DESC direction. The bug has been present since 21.1 and is now fixed. Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Contributor
|
Build failed (retrying...): |
Contributor
This was referenced Oct 7, 2025
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 83d950e to blathers/backport-release-24.1-154914: POST https://api.github.com/repos/yuzefovich/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch release-24.1 failed. See errors above. error creating merge commit from 83d950e to blathers/backport-release-24.3-154914: POST https://api.github.com/repos/yuzefovich/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch release-24.3 failed. See errors above. error creating backport branch refs/heads/blathers/backport-release-25.3-154914: POST https://api.github.com/repos/yuzefovich/cockroach/git/refs: 422 Reference already exists [] Backport to branch release-25.3 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Member
Author
|
blathers backport release-25.4 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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 commit fixes a bug that was added long time ago when we added support for inverted joins on multi-column inverted indexes (i.e. such indexes that have a prefix of "forward" columns that we can constrain using an equality filter) in d3f9a8b. In particular, in that change we added tricky logic which prepended the prefix to the encoded inverted key; however, we then didn't change how we use the
span.Builder. That guy assumes that it receives datums for each index column (both prefix and inverted) whereas we only provided the inverted bytes (with the prepended prefix). This led to us being confused about which key column is being encoded during span construction.Consider an example where we have one prefix key column like
i INT DESC(followed by the inverted column). In this case, when callingMakeKeyFromEncDatumswe'd try to encode the single EncDatum using the DESC key direction. The EncDatum internally stores the prefix + inverted key using ASC key direction, so we'd try to decode it and fail - because of the prepended prefix. After this patch, we'll correctly only use a single column in the call toMakeKeyFromEncDatums.Note that if prefix columns had only ASC directions, then we would simply get lucky. Namely, since we have EncDatumRow with a single EncDatum AND that EncDatum already has encoded value of ASC direction stored, we'd just reuse it and exit.
The code here is quite difficult to follow and perhaps it could be refactored (e.g. the comment on
SpansFromInvertedSpanssays that multi-column inverted indexes are expected to use non-nil constraint, yet in the inverted joiner we always pass nil), but I chose a more targetted fix that seems reasonable.Additionally, to simplify the change a bit, I inlined the code for
rowenc.MakeSpanFromEncDatumsinto two call sites (which also allows us to avoid constructing the unnecessary end key for the inverted spans case).Fixes: #153499.
Release note (bug fix): Previously, CockroachDB would hit an internal error when performing an inverted join using an inverted index in which the first prefix column had DESC direction. The bug has been present since 21.1 and is now fixed.