Implement SVSIndex::getDataByLabel (MOD-17706) - #1033
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1033 +/- ##
==========================================
- Coverage 97.41% 97.40% -0.01%
==========================================
Files 141 141
Lines 8688 8698 +10
==========================================
+ Hits 8463 8472 +9
- Misses 225 226 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // Whether the buffer held it, measured rather than read off emptiness, so the tier | ||
| // decision does not depend on an assertion that only exists in test builds. | ||
| if (this->backendIndex->isMultiValue() || vectors_output.size() == before_flat) { | ||
| std::shared_lock<std::shared_mutex> main_lock(this->mainIndexGuard); |
There was a problem hiding this comment.
For a compressed SVS backend with no buffer contribution, this acquires mainIndexGuard even though the result must be empty. It can wait behind a batch’s exclusive addVectors lock, then immediately return nothing from SVSIndex::getDataByLabel. While waiting, it also holds flatIndexGuard shared, potentially delaying buffer writes.
Could we resolve svs_backend before acquiring the main lock and return early when isCompressed() && vectors_output.size() == before_flat?
The multi-value case with a buffer contribution should retain the locked isLabelExists check, since we still need to determine whether the buffer represents the complete label.
| std::vector<OutputElement> vec_data(this->getStoredDataSize() / sizeof(OutputElement)); | ||
| const char *data_ptr = reinterpret_cast<const char *>(indexed_span.data()); | ||
| std::memcpy(vec_data.data(), data_ptr, this->getStoredDataSize()); |
There was a problem hiding this comment.
Consider adding a same type optimization (if OutputElement and DataType),
We can create vec_data directly in vectors_output.
Something like:
auto append_datum = [&](auto indexed_span) {
if constexpr (std::is_same_v<OutputElement, DataType>) {
vectors_output.emplace_back(indexed_span.begin(), indexed_span.end());
} else {
// Existing implementation, preserved for FP16 and byte output.
std::vector<OutputElement> vec_data(
this->getStoredDataSize() / sizeof(OutputElement));
std::memcpy(vec_data.data(), indexed_span.data(), this->getStoredDataSize());
vectors_output.push_back(std::move(vec_data));
}
};
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fa5b616. Configure here.
SVSIndex::getDataByLabel was a stub that always reported nothing, so a tiered SVS index could never learn a label's stored vectors -- the special case documented on VecSimTieredIndex::getDataByLabel and the TODO left on the stub itself. It now reads the stored vectors directly from the underlying SVS index (single-value via get_datum, multi-value via get_label_to_external_lookup + get_parent_index), refusing only for compressed/LeanVec-reduced storage, per storage_traits_t::is_compressed() -- the same "report nothing rather than dequantize" rule the HNSW implementations already follow. That lets VecSimTieredIndex::getDataByLabel drop its SVSIndexBase bypass and read both tiers unconditionally. The read logic is shared with the existing test-only getStoredVectorDataByLabel via a new private appendStoredDataByLabel helper, so the single/multi-value branching and the raw memcpy live in one place. Tests cover both isMulti and quant_bits variants: compressed storage reports nothing, uncompressed single- and multi-value report the stored vectors in insertion order, and an absent label reports nothing. The tiered test now expects the SVS backend to answer once a vector has been ingested into it, plus the existing flat-buffer coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
clang-format flagged two lines in appendStoredDataByLabel's comment and the vec_data construction added in the previous commit.
Codex review on this PR: removing the SVS bypass in VecSimTieredIndex::getDataByLabel made it always read the flat buffer, then the backend for multi-value labels. A compressed SVS backend appends nothing regardless of whether it holds part of the label -- it cannot report values, not "doesn't have any" -- so a multi-value label split across a non-empty buffer contribution and such a backend came back as just the buffer's subset, indistinguishable from the complete answer. Unlike HNSW, whose tiered backend can never be quantized, SVS's routinely is, so this case is reachable in practice. `SVSIndexBase::isLabelExists` is metadata, not the value read compression rules out, so it is cheap to check membership before trusting the buffer alone: if the backend is compressed and holds part of this label, the whole answer becomes "cannot tell" instead of a partial one -- the same rule `SVSIndex::getDataByLabel` already applies within a single tier. Covered by a new multi-value + Quant_8 combination (not in SVSDataTypeSet, which only pairs compression with single-value) that ingests one vector into the compressed backend and leaves a second one for the same label in the flat buffer, then asserts `getDataByLabel` reports nothing. Checked against a build with the fix disabled before trusting it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review comment on this PR: appendStoredDataByLabel's byte-copy path is only needed when OutputElement differs from the stored span's element type (DataType) -- FP16's vecsim_dt mapping, or the test-only char output. For FLOAT32/FLOAT64, both getDataByLabel's real callers, OutputElement *is* DataType, so the span can construct the output vector directly via its begin()/end() iterators instead of a raw memcpy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review comment on this PR: with no buffer contribution, a compressed SVS backend's getDataByLabel always appends nothing regardless of whether it holds the label, so the eventual answer is already known empty. Waiting on mainIndexGuard behind a batch update's exclusive hold just to confirm that is pure overhead -- and the wait happens while still holding flatIndexGuard shared, which a buffer writer needs exclusively. svs_backend is now resolved before either lock, and a compressed backend with no buffer contribution returns early right after the frontend read. The multi-value, buffer-contribution case keeps the locked isLabelExists check, since that's the one case where the buffer alone isn't known to be the complete answer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
01f5778 to
ee9745e
Compare

Describe the changes in the pull request
SVSIndex::getDataByLabelwas a stub that always reported nothing whereVecSimTieredIndex::getDataByLabelhad a bypass for SVS backend.It now reads the stored vectors directly from the underlying SVS index (single-value via
get_datum, multi-value viaget_label_to_external_lookup+get_parent_index), refusing only forcompressed/LeanVec-reduced storage (
storage_traits_t::is_compressed()) -- where it reports nothing.The read logic is shared with the existing test-only
getStoredVectorDataByLabelvia a new privateappendStoredDataByLabelhelper, so the single/multi-value branching and the raw memcpy live in oneplace.
Tiered read logic
VecSimTieredIndex::getDataByLabelalways reads the flat buffer first, then decides whether thebackend also needs reading: for a single-value label the buffer alone is the whole answer once it
has contributed anything, but a multi-value label's vectors are routinely split across the tiers
while an ingest job is pending, so the backend is always consulted too in that case.
A compressed SVS backend complicates that decision: it appends nothing from
getDataByLabelregardless of whether it holds part of the label -- it cannot report values, which is not the
same as "doesn't have any". Unlike HNSW, whose tiered backend can never be quantized, SVS's
routinely is, so a multi-value label split between a non-empty buffer contribution and such a
backend would otherwise come back as just the buffer's subset, indistinguishable from the complete
answer. Before trusting that subset, the tiered read checks backend membership via
SVSIndexBase::isLabelExists-- cheap because it's metadata, not the value read compression rulesout -- and if the backend is compressed and holds part of the label, discards the buffer's
contribution too and reports nothing for the whole label, the same rule
SVSIndex::getDataByLabelalready applies within a single tier.
Which issues this PR fixes
Main objects this PR modified
src/VecSim/algorithms/svs/svs.hsrc/VecSim/vec_sim_tiered_index.htests/unit/test_svs.cpptests/unit/test_svs_tiered.cppMark if applicable
Note
Medium Risk
Changes observable
getDataByLabelbehavior for SVS and tiered SVS (including RediSearch “holds vectors” paths); logic for compressed backends and multi-value labels split across tiers is easy to get wrong.Overview
Implements
SVSIndex::getDataByLabelso uncompressed SVS indexes return the stored vectors for a label (single-value viaget_datum, multi-value via label lookup + parent index). Compressed or LeanVec-reduced indexes still append nothing, matching HNSW—no dequantized bytes that could fool equality checks.The read path is centralized in a new
appendStoredDataByLabelhelper, also used by the test-onlygetStoredVectorDataByLabel, with compression gated onstorage_traits_t::is_compressed()instead of separate quant flags.Tiered
getDataByLabelno longer skips all backend reads for SVS. It always consults the flat buffer first, then the backend when needed (multi-value or label not fully in the buffer). For a compressed SVS backend, if the buffer contributed vectors but the backend also holds that label (isLabelExists), the tiered read drops the buffer result and returns nothing so a partial subset is not mistaken for the full label.Unit tests cover uncompressed vs compressed, single vs multi, absent labels, tiered flat-only reads, tiered backend reads after ingest, and the compressed split-label case.
Reviewed by Cursor Bugbot for commit ee9745e. Bugbot is set up for automated code reviews on this repo. Configure here.