-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CNDB-12407: lazily load token in PrimaryKeyWithSource #1500
base: main
Are you sure you want to change the base?
Conversation
Also, add sstable overlap check to avoid certain lookups.
src/java/org/apache/cassandra/index/sai/disk/PrimaryKeyWithSource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice !
we should add some unit tests to cover the new code
I pushed this up to see what tests fail, if any, and to see the code coverage. I'll modify once I get those reports back. |
The bug here was caught by testHybridSearchHoleInClusteringColumnOrdering. It failed because the PartitionKey-only primary key said it was equal to one key but not another key because of the order of operations in the PrimaryKeyListUtil logic. One solution could have been to update the way that findBoundaryIndex works, but I think a more robust solution is to just load a fully qualified min/max PrK since those are used for boundary checks anyway.
Now that we have fully qualified PrK objects as boundaries, we can remove the check for the entry at the next index.
The min/max partition key is not always a pointer to the segment metadata's min/max sstable row id. I thought it was. Removing that assertion.
Quality Gate passedIssues Measures |
❌ Build ds-cassandra-pr-gate/PR-1500 rejected by Butler22 new test failure(s) in 8 builds Found 22 new test failuresShowing only first 15 new test failures
Found 51 known test failures |
Also, add sstable overlap check to avoid certain
lookups.
What is the issue
This is a possible fix for https://github.com/riptano/cndb/issues/12407.
What does this PR fix and why was it fixed
This PR skips loading the PrimaryKey's token in the case where the sstables/memtables do not overlap.
It is implemented via a subtle change to the
PrimaryKeyWithSource
class that only loads the token info when the token is needed.Checklist before you submit for review
NoSpamLogger
for log lines that may appear frequently in the logs