CNDB-12922: Implement rerank_k in SAI ANN queries #1562
Conversation
This file contains 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
Before this, we had minor inconcistencies where the in memory and the on disk indexes diverged. This might have been intentional, but I'm not certain, so changing this here to force the discussion in the PR review. Now, everything is consistently rerankK when estimating the cost to search the graph in order to determine if brute force is cheaper.
Checklist before you submit for review
|
Comment on lines
171
to
174
| // Default to no warning and failure at 4 times the maxTopK value | ||
| int maxTopK = CassandraRelevantProperties.SAI_VECTOR_SEARCH_MAX_TOP_K.getInt(); | ||
| enforceDefault(sai_ann_rerank_k_warn_threshold, v -> sai_ann_rerank_k_warn_threshold = v, -1, -1); | ||
| enforceDefault(sai_ann_rerank_k_failure_threshold, v -> sai_ann_rerank_k_failure_threshold = v, 4 * maxTopK, 4 * maxTopK); |
There was a problem hiding this comment.
Another approach could be to cap the ratio of LIMIT to rerank_k.
adelapena
reviewed
Feb 12, 2025
adelapena
reviewed
Feb 13, 2025
test/unit/org/apache/cassandra/guardrails/GuardrailSAIAnnRerankKTest.java
Outdated
Show resolved
Hide resolved
|
@michaeljmarshall for the CNDB PR, besides the messaging version bump, we will need to add the new guardrail to this test (I learned it the hard way) |
This can be reverted later when we support non-positive numbers, which will mean that we can do a rerankless search.
Comment on lines
+82
to
+86
| public static ANNOptions fromMap(String keyspace, Map<String, String> map) | ||
| { | ||
| assert keyspace != null; | ||
| // ensure that all nodes in the cluster are in a version that supports ANN options, including this one | ||
| Set<InetAddressAndPort> badNodes = MessagingService.instance().endpointsWithVersionBelow(MessagingService.VERSION_DS_11); | ||
| Set<InetAddressAndPort> badNodes = MessagingService.instance().endpointsWithVersionBelow(keyspace, MessagingService.VERSION_DS_11); |
There was a problem hiding this comment.
@adelapena - what do you think about moving this validation to the validate method? It seems odd to pass the keyspace in to this method since it is only needed for validation and not for building the object from the map. I left it as is for now since it is a minor detail.
|
@adelapena - will you please re-review this PR? Thanks! |
adelapena
approved these changes
Feb 24, 2025
test/unit/org/apache/cassandra/guardrails/GuardrailSAIAnnRerankKTest.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/guardrails/GuardrailSAIAnnRerankKTest.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/guardrails/GuardrailSAIAnnRerankKTest.java
Outdated
Show resolved
Hide resolved
|
❌ Build ds-cassandra-pr-gate/PR-1562 rejected by Butler3 new test failure(s) in 7 builds Found 3 new test failures
Found 15 known test failures |
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
Fixes https://github.com/riptano/cndb/issues/12922
What does this PR fix and why was it fixed
This PR follows up on #1525 by integrating the CQL configured
rerank_kin to SAI so that the ANN query can take the user input and let it influence query execution.Key changes:
MessagingServicetoVERSION_DS_11MessagingService.instance().endpointsWithVersionBelowby using thekeyspacevariant, which is necessary for CNDB.rerank_kis used as the graph'srerankKparameter. The only detail worth mentioning here is that we ignore the segment proportionality computation ifrerank_kis provided. This diverges from the smart default design, but not too greatly.rerank_kto prevent it from exceeding 4 times thecassandra.sai.vector_search.max_top_k. This is debatable and easily changed, so please let me know if we want something different.rerankKvalue instead of thelimit. I am pretty sure this is correct, but please review closely.Instead of validating
rerank_k's value within the query, I added a test that ensures that as we increase the rerank k, we increase the recall.CNDB pr: https://github.com/riptano/cndb/pull/13095