CNDB-13696 fix empty iterator access in BM25 search on partial SSTable #1691
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
Checklist before you submit for review
|
k-rus
force-pushed
the
rf-13667-reproduce-different-bm25-order
branch
from
3d31438 to
04e3862
Compare
pkolaczk
approved these changes
Apr 15, 2025
k-rus
changed the title
k-rus
changed the title
Adds tests to split data into two segments and demonstrate that the BM25 ordering is different than single table. Currently the test is broken due to a bug in BM25 implementation and this commit reproduces it.
Fixes the bug that if SSTable contains part of the data, it results in reading from an empty iterator.
k-rus
force-pushed
the
rf-13667-reproduce-different-bm25-order
branch
from
04e3862 to
3c3b69c
Compare
|
✔️ Build ds-cassandra-pr-gate/PR-1691 approved by ButlerApproved by Butler |
| @@ -168,6 +168,9 @@ private Cell<?> readColumn(SSTableReader sstable, PrimaryKey primaryKey) | |||
| var slices = Slices.with(indexContext.comparator(), Slice.make(primaryKey.clustering())); | |||
| try (var rowIterator = sstable.iterator(dk, slices, columnFilter, false, SSTableReadsListener.NOOP_LISTENER)) | |||
| { | |||
| // primaryKey might not belong to this sstable, thus the iterator will be empty | |||
| if (rowIterator.isEmpty()) | |||
There was a problem hiding this comment.
Nice fix. Adding a semi-related observation. If we end up seeing a lot of object allocations associated with the Slices which are O(n) for results matched in the WHERE clause, it might be worth finding a way to use sstable.couldContain(dk), which checks the bloom filter, to reduce allocations for keys not in the sstable. If you look in the implementation of the iterator method, you'll see how it already does that for us, but it's after we've already created our slice object.
There was a problem hiding this comment.
Good point. I will not be surprised if there are more places for such improvements.
michaeljmarshall
pushed a commit
that referenced
this pull request
Apr 18, 2025
#1691) Executing a query with BM25 search and a condition on partial SSTable results in empty iterator access error. And there was no test with storing data in segments. The PR implements BM25 search tests with splitting data into two tables. This reproduced this bug, CNDB-13696, and demonstrates current confusion on the BM25 ordering result to be fixed by CNDB-13553. This PR adds a check for empty iterator created for a PK belonging to another segment. This fixes the bug of trying to get the first element of an empty iterator. (cherry picked from commit 167a98c)
djatnieks
pushed a commit
that referenced
this pull request
Apr 22, 2025
#1691) Executing a query with BM25 search and a condition on partial SSTable results in empty iterator access error. And there was no test with storing data in segments. The PR implements BM25 search tests with splitting data into two tables. This reproduced this bug, CNDB-13696, and demonstrates current confusion on the BM25 ordering result to be fixed by CNDB-13553. This PR adds a check for empty iterator created for a PK belonging to another segment. This fixes the bug of trying to get the first element of an empty iterator.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What is the issue
Executing a query with BM25 search and a condition on partial SSTable results in empty iterator access error. And there was no test with storing data in segments.
What does this PR fix and why was it fixed
The PR implements BM25 search tests with splitting data into two tables. This reproduced this bug, CNDB-13696, and demonstrates current confusion on the BM25 ordering result to be fixed by CNDB-13667.
This PR adds a check for empty iterator created for a PK belonging to another segment. This fixes the bug of trying to get the first element of an empty iterator.
Fixes https://github.com/riptano/cndb/issues/13696, fixes https://github.com/riptano/cndb/issues/13667
It's based on #1688, which fixes https://github.com/riptano/cndb/issues/13671
It's planned to be two commits on top of the commit in #1688.(it's okay to squash as the test reproduces the issue)