-
Notifications
You must be signed in to change notification settings - Fork 19
Enabler for proper de-duplication of query results in tiered index with compression #689
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
Enabler for proper de-duplication of query results in tiered index with compression #689
Conversation
…ions, allowing inheriting classes to control whether the merge should use sets. use it with withSet =true in svs add test to svs
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## rfsaliev/scalable-vector-search-tiered #689 +/- ##
==========================================================================
- Coverage 96.43% 96.40% -0.04%
==========================================================================
Files 112 113 +1
Lines 6795 6837 +42
==========================================================================
+ Hits 6553 6591 +38
- Misses 242 246 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
// ID 54: closer in SVS, farther in flat — expect to return SVS version | ||
GenerateAndAddVector<TEST_DATA_T>(svs_index, dim, ids[0], res_values[0]); | ||
GenerateAndAddVector<TEST_DATA_T>(flat_index, dim, ids[0], 4); |
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.
As I understand, flat index should be prioritized in this case:
- user adds a vector with a value version_1 (moved to backend)
- user overrides the vector with a value version_2 (not yet moved to backend but kept in flat)
- user expects that version_1 is overridden and forgotten and will never appear in query results
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.
- This test simulates discrepancies in scores due to quantization differences, not synchronization issues.
- The scenario you described is valid. However, in general, the flat index result can also be selected if it yields a better score for the same label compared to the SVS index—again, assuming updates are properly handled and the score is calculated for the same vector version.
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.
Let's assume the following:
- user adds a vector with label 1 which is close to further query
- user adds 1000 other vectors
- user overrides the vector with label 1 with a value which is farther than all other 1000 vectors
- user calls topK query where k=10
- user does not expect the vector with label 1 in query results, but if the first version of the vector is kept in backend, user will receive it.
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.
then the implementation is wrong.
The vector should have been marked as deleted in the backend index during the update, and should not be part of the results returned from the backend index.
// To avoid duplicates in the final result, we must use withSet=true because backend vectors | ||
// are quantized, | ||
// and may produce different scores than the flat index for the same label. | ||
return this->template topKQueryImp<true>(queryBlob, k, queryParams); |
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.
TODO: set to true only if quantization is enabled.
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.
@rfsaliev Could you please guide me on how to check whether the SVS index performs quantization (e.g., LVQ) in a way that minimizes runtime overhead? Ideally, I’d like to avoid adding dynamic casts or branching logic if possible.
// To avoid duplicates in the final result, we must use withSet=true because backend vectors | ||
// are quantized, | ||
// and may produce different scores than the flat index for the same label. | ||
return this->template rangeQueryImp<true>(queryBlob, radius, queryParams, order); |
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.
same
replaced with #692 - so it can be merged to main directly |
Describe the changes in the pull request
A clear and concise description of what the PR is solving.
Which issues this PR fixes
Main objects this PR modified
Mark if applicable