Skip to content

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

Conversation

meiravgri
Copy link
Collaborator

Describe the changes in the pull request

A clear and concise description of what the PR is solving.

Which issues this PR fixes

  1. #...
  2. MOD...

Main objects this PR modified

  1. ...
  2. ...

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

meiravgri added 2 commits June 3, 2025 10:42
…ions, allowing inheriting classes to control whether the merge should use sets.

use it with withSet  =true in svs
add test to svs
Copy link

codecov bot commented Jun 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.40%. Comparing base (9e77a09) to head (e3a9c96).
Report is 9 commits behind head on rfsaliev/scalable-vector-search-tiered.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines +432 to +434
// 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);
Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This test simulates discrepancies in scores due to quantization differences, not synchronization issues.
  2. 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.

Copy link
Collaborator

@rfsaliev rfsaliev Jun 3, 2025

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.

Copy link
Collaborator Author

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);
Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@alonre24 alonre24 changed the title Meiravg_topk_withset_svs Proper de-duplication of query results in tiered index with compression Jun 5, 2025
@alonre24 alonre24 changed the title Proper de-duplication of query results in tiered index with compression Enabler for proper de-duplication of query results in tiered index with compression Jun 5, 2025
@alonre24
Copy link
Collaborator

replaced with #692 - so it can be merged to main directly

@alonre24 alonre24 closed this Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants