CNDB-15638: Fix handling mixed key types in SAI iterators #2047
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
This commit fixes multiple issues with KeyRangeIterator implementations occasionally skipping or emitting duplicate keys when working on a mix of primary keys with empty / non-empty clusterings. This situation is possible while scanning tables with static columns or when some indexes are partition-aware (e.g. version AA) and others have been updated to a row-aware version (e.g. DC or EC). Due to those bugs, users could get incorrect results from SAI queries, e.g. results containing duplicated rows, duplicated partitions or even missing rows. The commit introduces extensive randomized property-based tests for KeyRangeUnionIterator and KeyIntersectionIterator. Previously, the tests did not test for keys with mixed empty/non-empty clusterings. Changes in KeyRangeUnionIterator: KeyRangeUnionIterator merges streams of primary keys in such a way that duplicates are removed. Unfortunately it does not properly account for the fact that if a key with an empty clustering meets a key with a non-empty clustering and the same partition key, we must always return the key with an empty clustering. A key with an empty clustering will always fetch the rows matched by any specific row key for the same partition, but the reverse is not true. The iterator implementation has been modified to always pick the key that matches more rows - a key with empty clustering wins over a key with non-empty clustering. Additionally, once a key with an empty clustering is emitted, no more keys in that partition are emitted. Changes in KeyRangeIntersectionIterator: Due to a very similar problem like in KeyRangeUnionIterator, KeyRangeIntersectionIterator could return either too few or too many keys, when keys with empty clusterings and keys with non-empty clusterings were present in the input key streams. In particular consider 2 input streams A and B with the following keys: A: 0: (1, Clustering.EMPTY) B: 0: (1, 1) 1: (1, 2) Key A.0 matches the whole partition 1. Therefore, the correct result of intersection are both keys of stream B. Unfortunately, the algorithm before this patch would advance both A and B iterators when emitting the first matching key. At the beginning of the second step, the iterator A would be already exhausted and no more keys would be produced. Finally key B.1 would be missing from the results. This patch fixes it by introducing two changes to the intersection algorithm: 1. A key with non-empty clustering wins over a key with empty clustering and same partition. 2. The selected highest key is not consumed while searching for the highest matching key, but that happens only after the search loop finds a match. Then we have more information which iterators would be moved to the next item. Iterators positioned at a key with an empty clustering can be advanced only after we run out of keys with non-empty clustering in the same partition or if there are no other keys with non-empty clustering. This patch also fixes another issue where we could return a less-specific key matching a full partition instead of a key matching one row: A: 0: (1, Clustering.EMPTY) B: 0: (1, 1) In that case the iterator returned a key with empty clustering, which would result in fetching and postfiltering many unnecessary rows. (cherry picked from commit d7b8944)
michaeljmarshall
changed the title
Checklist before you submit for review
|
eolivelli
previously approved these changes
Oct 9, 2025
The PrimaryKeyWithSource class has been present for two years in the code base as an optimization for hybrid vector workloads, which have to materialize many primary keys in the search-then-sort query path. However, the logic is invalid for version aa (because we have the bug where compacted sstables write per row, not per partition) and it is also invalid for static columns. This commit avoids creation of PrimaryKeyWithSource in those cases. (cherry picked from commit e942cae)
eolivelli
dismissed
their stale review
Patch is not exactly the same as https://github.com/datastax/cassandra/pull/2037/files
I don't fell comfortable in approving.
We need Piotr's eyes here
pkolaczk
approved these changes
Oct 10, 2025
|
|
Butler seems to be wrong on this one. It highlights some old regressions we fixed. I checked the last CI run manually and the only failure there is BinLogTest (which is known to be flaky and I must try to fix it one more time). |
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.
Fixes: https://github.com/riptano/cndb/issues/15638
Cherry-picks: #2037
This commit fixes multiple issues with KeyRangeIterator implementations
occasionally skipping or emitting duplicate keys when working on
a mix of primary keys with empty / non-empty clusterings.
This situation is possible while scanning tables with static columns
or when some indexes are partition-aware (e.g. version AA) and
others have been updated to a row-aware version (e.g. DC or EC).
Due to those bugs, users could get incorrect results from SAI queries,
e.g. results containing duplicated rows, duplicated partitions or
even missing rows.
The commit introduces extensive randomized property-based tests for
KeyRangeUnionIterator and KeyIntersectionIterator. Previously,
the tests did not test for keys with mixed empty/non-empty clusterings.
Changes in KeyRangeUnionIterator:
KeyRangeUnionIterator merges streams of primary keys in such a way that
duplicates are removed. Unfortunately it does not properly account
for the fact that if a key with an empty clustering meets a key
with a non-empty clustering and the same partition key, we must
always return the key with an empty clustering. A key with an empty
clustering will always fetch the rows matched by any specific row
key for the same partition, but the reverse is not true.
The iterator implementation has been modified to always pick the
key that matches more rows - a key with empty clustering wins
over a key with non-empty clustering. Additionally, once a key
with an empty clustering is emitted, no more keys in that partition
are emitted.
Changes in KeyRangeIntersectionIterator:
Due to a very similar problem like in KeyRangeUnionIterator,
KeyRangeIntersectionIterator could return either too few or
too many keys, when keys with empty clusterings and keys
with non-empty clusterings were present in the input key streams.
In particular consider 2 input streams A and B with the following
keys:
A:
0: (1, Clustering.EMPTY)
B:
0: (1, 1)
1: (1, 2)
Key A.0 matches the whole partition 1. Therefore, the correct result
of intersection are both keys of stream B. Unfortunately, the algorithm
before this patch would advance both A and B iterators when emitting
the first matching key. At the beginning of the second step,
the iterator A would be already exhausted and no more keys would
be produced. Finally key B.1 would be missing from the results.
This patch fixes it by introducing two changes to the intersection
algorithm:
A key with non-empty clustering wins over a key with
empty clustering and same partition.
The selected highest key is not consumed while searching
for the highest matching key, but that happens only after the
search loop finds a match. Then we have more information
which iterators would be moved to the next item. Iterators positioned
at a key with an empty clustering can be advanced only after
we run out of keys with non-empty clustering in the same partition
or if there are no other keys with non-empty clustering.
This patch also fixes another issue where we could return
a less-specific key matching a full partition instead of a key
matching one row:
A:
0: (1, Clustering.EMPTY)
B:
0: (1, 1)
In that case the iterator returned a key with empty clustering,
which would result in fetching and postfiltering many unnecessary rows.
(cherry picked from commit d7b8944)