diff --git a/src/VecSim/algorithms/svs/svs.h b/src/VecSim/algorithms/svs/svs.h index ec15cabbd..1e124519f 100644 --- a/src/VecSim/algorithms/svs/svs.h +++ b/src/VecSim/algorithms/svs/svs.h @@ -16,6 +16,7 @@ #include #include #include +#include #include #include "svs/index/vamana/dynamic_index.h" @@ -745,6 +746,49 @@ class SVSIndex : public VecSimIndexAbstract, 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 + void appendStoredDataByLabel(labelType label, + std::vector> &vectors_output) const { + if (!impl_) { + return; + } + auto append_datum = [&](auto indexed_span) { + if constexpr (std::is_same_v) { + // 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 vec_data(this->getStoredDataSize() / + sizeof(OutputElement)); + const char *data_ptr = reinterpret_cast(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: @@ -760,75 +804,33 @@ class SVSIndex : public VecSimIndexAbstract, fl void fitMemory() override {} size_t indexMetaDataCapacity() const override { return this->indexCapacity(); } std::vector> 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> 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(indexed_span.data()); - std::vector 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(indexed_span.data()); - std::vector 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>> &vectors_output) const override { - this->log(VecSimCommonStrings::LOG_DEBUG_STRING, - "getDataByLabel: not implemented for SVS, reporting no stored vectors for " - "label %zu", - static_cast(label)); + if constexpr (storage_traits_t::is_compressed()) { + return; + } else { + appendStoredDataByLabel(label, vectors_output); + } } }; diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index f67e0a649..1497ffba3 100644 --- a/src/VecSim/vec_sim_tiered_index.h +++ b/src/VecSim/vec_sim_tiered_index.h @@ -19,7 +19,7 @@ #include #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 @@ -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 @@ -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> &vectors_output) const { #ifdef BUILD_TESTS @@ -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 - // 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(this->backendIndex) == nullptr; + const auto *svs_backend = dynamic_cast(this->backendIndex); + const bool backend_cannot_report = svs_backend && svs_backend->isCompressed(); +#endif + + std::shared_lock 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 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 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 main_lock(this->mainIndexGuard); +#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); } } diff --git a/tests/unit/test_svs.cpp b/tests/unit/test_svs.cpp index 27b381327..144e1562f 100644 --- a/tests/unit/test_svs.cpp +++ b/tests/unit/test_svs.cpp @@ -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(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 v1(dim), v2(dim); + GenerateVector(v1.data(), dim, 1.0f); + GenerateVector(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 *>(index); - ASSERT_NE(typed, nullptr); - std::vector> stored; - typed->getDataByLabel(1, stored); - EXPECT_TRUE(stored.empty()) << "SVS cannot report stored vectors, and must not pretend to"; + auto *typed = dynamic_cast *>(index); + ASSERT_NE(typed, nullptr) << case_msg; + + std::vector> 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> 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) { diff --git a/tests/unit/test_svs_tiered.cpp b/tests/unit/test_svs_tiered.cpp index 9a72787a2..8ff9d7d38 100644 --- a/tests/unit/test_svs_tiered.cpp +++ b/tests/unit/test_svs_tiered.cpp @@ -180,6 +180,17 @@ using SVSBasicDataTypeSet = TYPED_TEST_SUITE(SVSTieredIndexTestBasic, SVSBasicDataTypeSet); +// A multi-value, compressed combination isn't in SVSDataTypeSet (compression there is only +// paired with single-value), but it is exactly the combination that can leave a label split +// across a non-empty flat buffer and a backend that cannot report its values. +template +class SVSTieredIndexTestCompressedMulti : public SVSTieredIndexTest {}; + +using SVSCompressedMultiDataTypeSet = + ::testing::Types>; + +TYPED_TEST_SUITE(SVSTieredIndexTestCompressedMulti, SVSCompressedMultiDataTypeSet); + TYPED_TEST(SVSTieredIndexTest, ThreadsReservation) { // Set thread_pool_size to 4 or actual number of available CPUs const auto num_threads = std::min(4U, getAvailableCPUs()); @@ -487,14 +498,13 @@ TYPED_TEST(SVSTieredIndexTest, CreateIndexInstance) { ASSERT_EQ(tiered_index->GetBackendIndex()->indexSize(), 1); } -// The tiered read reports nothing when the backend cannot report, and does not fall back to the -// flat buffer. SVS has no per-label read of its stored vectors (MOD-17706), and the buffer alone -// would be a partial answer for a multi-value label split across the tiers -- a caller cannot tell -// a subset from the whole, so nothing is the only honest answer. -// -// The vector is deliberately left in the flat buffer, where the frontend *could* have answered: -// that is what makes this test fail if the SVS check is removed or the fallback reinstated. -TYPED_TEST(SVSTieredIndexTestBasic, getDataByLabelReportsNothingForSvsBackend) { +// A label still sitting in the flat buffer must be readable from there regardless of what the +// SVS backend can or cannot report: the buffer is a plain, uncompressed BruteForceIndex, so +// nothing about the backend's own read limitations (MOD-17706) applies to it. This used to fail +// -- the backend's inability to report gated the flat-buffer read too, so a vector written +// recently enough to still be buffered (exactly when a document is most likely to be written +// again) was invisible to `getDataByLabel`. +TYPED_TEST(SVSTieredIndexTestBasic, getDataByLabelReadsFlatBufferForSvsBackend) { const size_t dim = 4; SVSParams params = {.type = TypeParam::get_index_type(), .dim = dim, .metric = VecSimMetric_L2}; VecSimParams svs_params = CreateParams(params); @@ -509,12 +519,79 @@ TYPED_TEST(SVSTieredIndexTestBasic, getDataByLabelReportsNothingForSvsBackend) { GenerateVector(vector, dim, 0); VecSimIndex_AddVector(tiered_index, vector, 0); ASSERT_EQ(tiered_index->GetFlatIndex()->indexSize(), 1) - << "premise: the label is in the flat buffer, which could have reported it"; + << "premise: the label is in the flat buffer"; + + std::vector> stored; + tiered_index->getDataByLabel(0, stored); + ASSERT_EQ(stored.size(), 1); + EXPECT_EQ(stored[0], std::vector(vector, vector + dim)); +} + +// Once the vector has been ingested into an uncompressed SVS backend, `getDataByLabel` now +// reads it from there too (MOD-17706) -- the backend read is no longer skipped for SVS. +TYPED_TEST(SVSTieredIndexTestBasic, getDataByLabelReadsSvsBackendWhenUncompressed) { + const size_t dim = 4; + SVSParams params = {.type = TypeParam::get_index_type(), .dim = dim, .metric = VecSimMetric_L2}; + VecSimParams svs_params = CreateParams(params); + auto mock_thread_pool = tieredIndexMock(); + + auto *tiered_index = this->CreateTieredSVSIndex(svs_params, mock_thread_pool); + ASSERT_INDEX(tiered_index); + + TEST_DATA_T vector[dim]; + GenerateVector(vector, dim, 0); + VecSimIndex_AddVector(tiered_index, vector, 0); + ASSERT_EQ(tiered_index->GetFlatIndex()->indexSize(), 1); + + // Ingest it into the SVS backend. + tiered_index->scheduleSVSIndexUpdate(); + mock_thread_pool.thread_iteration(); + ASSERT_EQ(tiered_index->GetFlatIndex()->indexSize(), 0); + ASSERT_EQ(tiered_index->GetBackendIndex()->indexSize(), 1); + + std::vector> stored; + tiered_index->getDataByLabel(0, stored); + ASSERT_EQ(stored.size(), 1); + EXPECT_EQ(stored[0], std::vector(vector, vector + dim)); +} + +// A compressed SVS backend appends nothing to `getDataByLabel` regardless of whether it holds +// part of the label, so a multi-value label split across a non-empty flat buffer and such a +// backend must not be reported as the buffer's subset alone -- that subset would look like the +// complete answer when the backend may hold the rest and simply can't say so. The whole label +// must report nothing instead, the same rule `SVSIndex::getDataByLabel` already applies within +// a single (compressed) tier. +TYPED_TEST(SVSTieredIndexTestCompressedMulti, getDataByLabelReportsNothingForCompressedSplitLabel) { + const size_t dim = 4; + SVSParams params = { + .type = TypeParam::get_index_type(), .dim = dim, .metric = VecSimMetric_L2, .multi = true}; + VecSimParams svs_params = CreateParams(params); + auto mock_thread_pool = tieredIndexMock(); + + // Force the tiered index to submit the update job on every insert. + auto *tiered_index = this->CreateTieredSVSIndex(svs_params, mock_thread_pool, 1, 1); + ASSERT_INDEX(tiered_index); + + // The label's first vector gets ingested into the compressed SVS backend... + TEST_DATA_T vector1[dim]; + GenerateVector(vector1, dim, 0); + VecSimIndex_AddVector(tiered_index, vector1, 0); + mock_thread_pool.thread_iteration(); + ASSERT_EQ(tiered_index->GetBackendIndex()->indexSize(), 1) + << "premise: the first vector reached the compressed SVS backend"; + + // ...and, because the update job only fires on the next insert, the label's second vector is + // still sitting in the flat buffer: the label is now split across both tiers. + TEST_DATA_T vector2[dim]; + GenerateVector(vector2, dim, 1); + VecSimIndex_AddVector(tiered_index, vector2, 0); + ASSERT_EQ(tiered_index->GetFlatIndex()->indexSize(), 1) + << "premise: the second vector is still in the flat buffer"; std::vector> stored; tiered_index->getDataByLabel(0, stored); EXPECT_TRUE(stored.empty()) - << "an SVS backend cannot report, so the tiered read reports nothing"; + << "the buffer's one vector must not be reported as if it were the whole label"; } TYPED_TEST(SVSTieredIndexTestBasic, ShrinkDuringScheduledUpdateIsDeferred) {