CNDB-15666: CNDB-15570: Fix handling mixed key types in SAI iterators #2077
Conversation
Checklist before you submit for review
|
|
Test failures look legit, something with the aa format? |
Yes, the failures are only happening with Extending the search results (cql So the row for I can't see (yet) how the iterators can be messing this up. While I investigate (there are changes to them from |
|
i'm going to add in CNDB-15776 here… |
michaelsembwever
force-pushed
the
mck-cndb-15666-main-5.0
branch
2 times, most recently
from
02916fe to
99c18fd
Compare
|
Looks like there are still some |
|
You need to include CNDB-14861 first. That should fix it. |
michaelsembwever
force-pushed
the
mck-cndb-15666-main-5.0
branch
2 times, most recently
from
ccf62dc to
e739a64
Compare
|
@pkolaczk , new failure here also related to |
|
That failure is related to missing a fix for resetting SAI format versions in our test framework. https://github.com/riptano/cndb/issues/15619 Looks like you already started porting it: The critical code fragment to make the test work is the following snippet in // update the index contexts for each keyspace
for (String keyspaceName : Schema.instance.getKeyspaces())
{
Keyspace keyspace = Keyspace.open(keyspaceName);
for (ColumnFamilyStore cfs : keyspace.getColumnFamilyStores())
{
SecondaryIndexManager sim = cfs.getIndexManager();
for (Index index : sim.listIndexes())
{
if (index instanceof StorageAttachedIndex)
{
StorageAttachedIndex sai = (StorageAttachedIndex)index;
IndexContext context = sai.getIndexContext();
Field field = IndexContext.class.getDeclaredField("primaryKeyFactory");
field.setAccessible(true);
field.set(context, version.onDiskFormat().newPrimaryKeyFactory(cfs.metadata().comparator));
}
}
}
}You can just paste this snippet at the end of the try block and the test will work fine. |
Thanks! I'm ok merging this PR without the THROWAWAY, knowing it's ready to be merged in soon after. |
djatnieks
left a comment
djatnieks
left a comment
There was a problem hiding this comment.
looks good.
VectorKeyRestrictedOnPartitionTest was reported by butler but it passed locally using this branch.
CNDB-15683: Fix incorrect results when querying mixed AA and EC indexes 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. CNDB-15683: Fix incorrect results when querying mixed AA and EC indexes (#2066) When row-aware and non-row-aware indexes are mixed, we now check the clustering index filter for all the keys that have clustering information, i.e. keys coming from the row-aware indexes. Earlier that check was accidentally disabled if at least one non-row-aware index was used by the query. That could cause retrieving rows that do not match the clustering condition of the query. Rebase notes: - includes CNDB-15683
michaelsembwever
force-pushed
the
mck-cndb-15666-main-5.0
branch
from
1744b66 to
b10647c
Compare
|
❌ Build ds-cassandra-pr-gate/PR-2077 rejected by Butler5 regressions found Found 5 new test failuresFound 6 known test failures |
https://github.com/riptano/cndb/issues/15666
https://github.com/riptano/cndb/issues/15776
Port into main-5.0 commit d7b8944
BLOCKED ON #2076