Skip to content

Implement SVSIndex::getDataByLabel (MOD-17706) #1033

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.

Sign up for GitHub

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

Merged
nonirosenfeldredis merged 5 commits into from
Sep 8, 2026
Merged

Implement SVSIndex::getDataByLabel (MOD-17706) #1033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
713635e
Implement SVSIndex::getDataByLabel (MOD-17706)
nonirosenfeldredis Sep 6, 2026
af46163
Satisfy check-format
nonirosenfeldredis Sep 6, 2026
c2b763f
Don't let a compressed SVS backend's silence look like a complete answer
nonirosenfeldredis Sep 6, 2026
d45ce88
Build getDataByLabel's output directly when the types already match
nonirosenfeldredis Sep 7, 2026
ee9745e
Skip mainIndexGuard when a compressed backend has nothing to add
nonirosenfeldredis Sep 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 56 additions & 54 deletions src/VecSim/algorithms/svs/svs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <memory>
#include <cassert>
#include <limits>
#include <type_traits>
#include <vector>

#include "svs/index/vamana/dynamic_index.h"
Expand Down Expand Up @@ -745,6 +746,49 @@ class SVSIndex : public VecSimIndexAbstract<svs_details::vecsim_dt<DataType>, fl
num_marked_deleted = 0;
}

private:
// appends the raw stored elements for `label` to `vectors_output`, one entry per stored vector
// -- zero if the label isn't held. Only meaningful when the caller has already excluded
// compressed storage; nothing here dequantizes.
template <typename OutputElement>
void appendStoredDataByLabel(labelType label,
std::vector<std::vector<OutputElement>> &vectors_output) const {
if (!impl_) {
return;
}
auto append_datum = [&](auto indexed_span) {
if constexpr (std::is_same_v<OutputElement, DataType>) {
// The span's element type already is `OutputElement` here, so build the
// output vector directly from it instead of a raw byte copy.
vectors_output.emplace_back(indexed_span.begin(), indexed_span.end());
} else {
// FP16 and the test-only byte output: `OutputElement` differs from the
// span's element type but is bit-identical size, so copy the raw bytes.
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());
vectors_output.push_back(std::move(vec_data));
}
};

if constexpr (isMulti) {
auto it = impl_->get_label_to_external_lookup().find(label);
if (it == impl_->get_label_to_external_lookup().end()) {
return;
}
for (auto external_id : it->second) {
append_datum(impl_->get_parent_index().get_datum(external_id));
}
} else {
if (!impl_->has_id(label)) {
return;
}
append_datum(impl_->get_datum(label));
}
}

public:
#ifdef BUILD_TESTS

private:
Expand All @@ -760,75 +804,33 @@ class SVSIndex : public VecSimIndexAbstract<svs_details::vecsim_dt<DataType>, fl
void fitMemory() override {}
size_t indexMetaDataCapacity() const override { return this->indexCapacity(); }
std::vector<std::vector<char>> getStoredVectorDataByLabel(labelType label) const override {

// For compressed/quantized indices, this function is not meaningful
// since the stored data is in compressed format and not directly accessible
if constexpr (QuantBits > 0 || ResidualBits > 0) {
if constexpr (storage_traits_t::is_compressed()) {
throw std::runtime_error(
"getStoredVectorDataByLabel is not supported for compressed/quantized indices");
} else {

std::vector<std::vector<char>> vectors_output;

if constexpr (isMulti) {
// Multi-index case: get all vectors for this label
auto it = impl_->get_label_to_external_lookup().find(label);
if (it != impl_->get_label_to_external_lookup().end()) {
const auto &external_ids = it->second;
for (auto external_id : external_ids) {
auto indexed_span = impl_->get_parent_index().get_datum(external_id);

// For uncompressed data, indexed_span should be a simple span
const char *data_ptr = reinterpret_cast<const char *>(indexed_span.data());
std::vector<char> vec_data(this->getStoredDataSize());
std::memcpy(vec_data.data(), data_ptr, this->getStoredDataSize());
vectors_output.push_back(std::move(vec_data));
}
}
} else {
// Single-index case
auto indexed_span = impl_->get_datum(label);

// For uncompressed data, indexed_span should be a simple span
const char *data_ptr = reinterpret_cast<const char *>(indexed_span.data());
std::vector<char> vec_data(this->getStoredDataSize());
std::memcpy(vec_data.data(), data_ptr, this->getStoredDataSize());
vectors_output.push_back(std::move(vec_data));
}

appendStoredDataByLabel(label, vectors_output);
return vectors_output;
}
}
svs::logging::logger_ptr getLogger() const override { return logger_; }
#endif

// TODO(MOD-17706): implement, and remove the SVSIndexBase check in
// VecSimTieredIndex::getDataByLabel that currently skips the backend read for SVS entirely.
//
// What it has to produce: the vectors stored under `label`, in the form the base contract
// describes -- the *stored* elements, i.e. after whatever preprocessing an insert applied --
// appending nothing when the label is absent, so the output size answers "is it held".
//
// Why it is empty today: SVS keeps vectors in the SVS library's own layout, quantized and for
// LeanVec dimensionality-reduced, and this wrapper has no per-label read of them.
//
// One rule to carry over from the HNSW implementations: report nothing rather than an
// approximation. They refuse when `isQuantized`, because a caller comparing a new value
// against the stored one byte for byte would read a dequantized reconstruction as a
// difference -- or worse, as a match. An SVS index that is quantized should answer the same
// way; only an unquantized one can answer truthfully.
//
// What it unblocks: the no-change-set path in RediSearch (`VectorIndex_HoldsVectors`), which
// is what serves JSON writes and background scans. Note that alone is not enough to make an
// SVS-backed vector field relabel -- `relabelVector` is also unimplemented for SVS, and both
// are needed.
// Same rule as the HNSW implementations: report nothing rather than an approximation.
// `storage_traits_t::is_compressed()` covers quantized (LVQ/scalar) and LeanVec-reduced
// storage alike -- nothing here dequantizes, so a caller comparing a new value against the
// stored one byte for byte would read a dequantized reconstruction as a difference, or
// worse, as a match. Only an uncompressed index can answer truthfully.
void getDataByLabel(
labelType label,
std::vector<std::vector<svs_details::vecsim_dt<DataType>>> &vectors_output) const override {
this->log(VecSimCommonStrings::LOG_DEBUG_STRING,
"getDataByLabel: not implemented for SVS, reporting no stored vectors for "
"label %zu",
static_cast<size_t>(label));
if constexpr (storage_traits_t::is_compressed()) {
return;
} else {
appendStoredDataByLabel(label, vectors_output);
}
}
};

Expand Down
69 changes: 30 additions & 39 deletions src/VecSim/vec_sim_tiered_index.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
#include <shared_mutex>

#if HAVE_SVS
// For the SVS special case in getDataByLabel; remove with it (MOD-17706).
// For the compressed-backend check in getDataByLabel.
#include "VecSim/algorithms/svs/svs.h"
#endif

Expand Down Expand Up @@ -118,9 +118,7 @@ class VecSimTieredIndex : public VecSimIndexInterface {
* @brief Get the vector elements stored under a label, in insertion order.
*
* Contract on `VecSimIndexAbstract::getDataByLabel`, including that `vectors_output` arrives
* empty, with two caveats a tiered index cannot
* avoid, both of which only ever make an equality-testing caller answer "different":
*
* empty, with two caveats:
* - The vectors are the buffer's followed by the backend's, which for a multi-value label
* split across the tiers is not insertion order.
* - An ingest job inserts into the backend before removing from the buffer, so a vector
Expand All @@ -132,13 +130,9 @@ class VecSimTieredIndex : public VecSimIndexInterface {
* the backend, as this used to, reports nothing for a vector written recently enough to
* still be buffered -- which is exactly when a document is most likely to be written again.
*
* `flatIndexGuard` is held across both reads, in the order `relabelVector` and
* `acquireSharedLocks` take: it cannot prevent a duplicate, but it does stop the buffer's
* copy being removed between them. The backend's own data guard is deliberately not taken
* here -- its `getDataByLabel` takes it, because a shared main lock does not exclude an
* ingest mutating under `indexDataGuard`. Same division as
* `computeUnifiedIndexLabelsSetUnsafe`, which holds the outer locks and lets `getLabelsSet`
* take the inner one.
* A compressed SVS backend cannot report its stored vectors as values. Hence, for
* a multi-value label that already got a buffer contribution, we check `isLabelExists`
* in the backend before trusting the buffer alone.
*/
void getDataByLabel(labelType label, std::vector<std::vector<DataType>> &vectors_output) const {
#ifdef BUILD_TESTS
Expand All @@ -147,37 +141,34 @@ class VecSimTieredIndex : public VecSimIndexInterface {
assert(vectors_output.empty() && "getDataByLabel expects an empty output vector");
#endif

// A quantized backend cannot report its stored vectors as values -- the stored form is
// compression plus metadata, and nothing here dequantizes -- so it would append nothing.
bool backend_can_report = true;
#if HAVE_SVS
Comment thread
cursor[bot] marked this conversation as resolved.
// TODO(MOD-17706): remove once SVSIndex::getDataByLabel reports real data. Removing it
// means deleting this block, the `backend_can_report` flag, and the guarded include of
// svs.h, then unwrapping the body below.
//
// Until then nothing is read at all for an SVS backend: the buffer alone would be a
// partial answer for a multi-value label split across the tiers, and a caller cannot tell
// a subset from the whole. Skipping the reads also avoids waiting on `mainIndexGuard`
// behind an SVS batch update to be told nothing.
//
// Here rather than as an override in TieredSVSIndex because this method is not virtual:
// `VecSimTieredIndex` derives from `VecSimIndexInterface`, which does not declare it, and
// callers reach it through a `VecSimTieredIndex *` (RediSearch dynamic_casts to exactly
// that), so a derived override would simply not be found. The type test is deliberately
// explicit rather than dressed up as a capability: it is a special case, not
// architecture.
backend_can_report = dynamic_cast<const SVSIndexBase *>(this->backendIndex) == nullptr;
const auto *svs_backend = dynamic_cast<const SVSIndexBase *>(this->backendIndex);
const bool backend_cannot_report = svs_backend && svs_backend->isCompressed();
#endif

std::shared_lock<std::shared_mutex> flat_lock(this->flatIndexGuard);
const size_t before_flat = vectors_output.size();
this->frontendIndex->getDataByLabel(label, vectors_output);
#if HAVE_SVS
// no relevant data in flat and backend cannot function
if (backend_cannot_report && vectors_output.size() == before_flat) {
return;
}
#endif
if (backend_can_report) {
std::shared_lock<std::shared_mutex> flat_lock(this->flatIndexGuard);
const size_t before_flat = vectors_output.size();
this->frontendIndex->getDataByLabel(label, vectors_output);
// 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);
this->backendIndex->getDataByLabel(label, vectors_output);
// continue to look the data in the backend
if (this->backendIndex->isMultiValue() || vectors_output.size() == before_flat) {
std::shared_lock<std::shared_mutex> main_lock(this->mainIndexGuard);

@dor-forer dor-forer Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@nonirosenfeldredis nonirosenfeldredis Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

#if HAVE_SVS
if (backend_cannot_report && vectors_output.size() > before_flat &&
svs_backend->isLabelExists(label)) {
// The buffer's contribution alone would look like the whole answer; report
// nothing instead, the same rule `SVSIndex::getDataByLabel` applies to a
// single tier.
vectors_output.resize(before_flat);
return;
}
#endif
this->backendIndex->getDataByLabel(label, vectors_output);
Comment thread
cursor[bot] marked this conversation as resolved.
}
}

Expand Down
94 changes: 73 additions & 21 deletions tests/unit/test_svs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2770,31 +2770,83 @@ TYPED_TEST(SVSTest, resolve_epsilon_runtime_params) {
VecSimIndex_Free(index);
}

// SVS keeps its vectors in the SVS library's own form -- quantized, and for LeanVec reduced --
// and does not hand them back, so `getDataByLabel` appends nothing. Per the contract on
// `VecSimIndexAbstract::getDataByLabel` an empty output reads as "cannot tell", which is the
// answer a caller comparing against stored data needs.
// An uncompressed SVS index can hand back exactly what it stored, in insertion order, per the
// contract on `VecSimIndexAbstract::getDataByLabel`. A compressed or LeanVec-reduced one keeps
// vectors in the SVS library's own form and does not dequantize, so it must append nothing --
// an empty output reads as "cannot tell", which is the answer a caller comparing against stored
// data needs; anything else risks a dequantized reconstruction reading as a match or a
// difference that isn't real. Checked across both the single- and multi-value index variants,
// since the multi case reads through a different lookup (`get_label_to_external_lookup` +
// `get_parent_index`) than the single case's direct `get_datum`.
//
// Pinned because the alternative is silence: while this was left to a default in the base class,
// a tiered SVS index reached a not-implemented stub through `VecSimTieredIndex::getDataByLabel`
// as soon as a vector had been ingested.
TEST(SVSTest, getDataByLabelReportsNothing) {
size_t dim = 4;
SVSParams params = {.type = VecSimType_FLOAT32, .dim = dim, .metric = VecSimMetric_L2};
VecSimParams index_params = CreateParams(params);
VecSimIndex *index = VecSimIndex_New(&index_params);
ASSERT_NE(index, nullptr);
// Also covers a label the index does not hold at all, which must report nothing regardless of
// compression.
TEST(SVSTest, getDataByLabel) {
// Limit VecSim log level to avoid printing too much information
VecSimIndexInterface::setLogCallbackFunction(svsTestLogCallBackNoDebug);
const size_t dim = 4;
const size_t present_label = 1;
const size_t absent_label = 999;

GenerateAndAddVector<float>(index, dim, 1);
ASSERT_EQ(VecSimIndex_IndexSize(index), 1);
for (bool is_multi : {false, true}) {
for (auto quant_bits : {VecSimSvsQuant_NONE, VecSimSvsQuant_Scalar, VecSimSvsQuant_8,
VecSimSvsQuant_4, VecSimSvsQuant_4x4, VecSimSvsQuant_4x8,
VecSimSvsQuant_4x8_LeanVec, VecSimSvsQuant_8x8_LeanVec}) {
SVSParams params = {
.type = VecSimType_FLOAT32,
.dim = dim,
.metric = VecSimMetric_L2,
.multi = is_multi,
.quantBits = quant_bits,
};
VecSimParams index_params = CreateParams(params);
VecSimIndex *index = VecSimIndex_New(&index_params);
if (index == nullptr) {
// Unsupported quant_bits on this build/CPU; `quant_modes` already covers that.
continue;
}
const std::string case_msg = "is_multi: " + std::to_string(is_multi) +
", quant_bits: " + std::to_string(quant_bits);

// `VecSimSvsQuant_NONE` is the only mode `isSVSQuantBitsSupported` ever falls back
// to (an unsupported non-NONE mode falls back to Scalar, never to NONE), so the
// requested mode alone tells us whether storage ended up compressed.
const bool is_compressed = quant_bits != VecSimSvsQuant_NONE;

std::vector<float> v1(dim), v2(dim);
GenerateVector<float>(v1.data(), dim, 1.0f);
GenerateVector<float>(v2.data(), dim, 2.0f);
ASSERT_EQ(VecSimIndex_AddVector(index, v1.data(), present_label), 1) << case_msg;
if (is_multi) {
// A second vector under the same label, to exercise the multi-value lookup path.
ASSERT_EQ(VecSimIndex_AddVector(index, v2.data(), present_label), 1) << case_msg;
}

auto *typed = dynamic_cast<VecSimIndexAbstract<float, float> *>(index);
ASSERT_NE(typed, nullptr);
std::vector<std::vector<float>> stored;
typed->getDataByLabel(1, stored);
EXPECT_TRUE(stored.empty()) << "SVS cannot report stored vectors, and must not pretend to";
auto *typed = dynamic_cast<VecSimIndexAbstract<float, float> *>(index);
ASSERT_NE(typed, nullptr) << case_msg;

std::vector<std::vector<float>> stored;
typed->getDataByLabel(present_label, stored);
if (is_compressed) {
EXPECT_TRUE(stored.empty())
<< "a compressed index must not report stored vectors: " << case_msg;
} else if (is_multi) {
ASSERT_EQ(stored.size(), 2) << case_msg;
EXPECT_EQ(stored[0], v1) << case_msg;
EXPECT_EQ(stored[1], v2) << case_msg;
} else {
ASSERT_EQ(stored.size(), 1) << case_msg;
EXPECT_EQ(stored[0], v1) << case_msg;
}

VecSimIndex_Free(index);
std::vector<std::vector<float>> stored_absent;
typed->getDataByLabel(absent_label, stored_absent);
EXPECT_TRUE(stored_absent.empty())
<< "a label the index does not hold must report nothing: " << case_msg;

VecSimIndex_Free(index);
}
}
}

TEST(SVSTest, quant_modes) {
Expand Down
Loading
Loading