From 9cc97ce32b1f158cdaf3821b431c587394115013 Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Wed, 26 Aug 2026 14:18:17 +0300 Subject: [PATCH 01/13] Let production code read a label's stored vectors `getDataByLabel` was declared and implemented inside `BUILD_TESTS`, so the only way production code could learn anything about a stored vector was its distance from another one -- which is not an equality test. Comparing a vector about to be written against the one already stored lets a caller replacing a document skip re-adding an unchanged vector, so the accessor moves out of the test-only guard. The base declaration becomes a defaulted virtual rather than pure: an index type that cannot hand its vectors back (SVS, whose implementation is a not-implemented stub) then needs no production definition, and an empty output says "cannot tell" in the same way an absent label does. That contract also removes two ways of asking about a label that does not exist. The single-value implementations called `labelToIdLookup.at()`, which throws, and the multi-value ones dereferenced `find()` without comparing against `end()`, which is undefined behaviour. Both now leave the output empty, which is what makes the output size usable as the answer to whether the index holds the label. Co-Authored-By: Claude Opus 5 (1M context) --- .../brute_force/brute_force_multi.h | 7 +++- .../brute_force/brute_force_single.h | 11 ++++-- src/VecSim/algorithms/hnsw/hnsw_multi.h | 6 ++- src/VecSim/algorithms/hnsw/hnsw_single.h | 11 ++++-- src/VecSim/vec_sim_index.h | 38 ++++++++++--------- 5 files changed, 45 insertions(+), 28 deletions(-) diff --git a/src/VecSim/algorithms/brute_force/brute_force_multi.h b/src/VecSim/algorithms/brute_force/brute_force_multi.h index 343faea6b..6bc9a668b 100644 --- a/src/VecSim/algorithms/brute_force/brute_force_multi.h +++ b/src/VecSim/algorithms/brute_force/brute_force_multi.h @@ -39,11 +39,12 @@ class BruteForceIndex_Multi : public BruteForceIndex { } std::unordered_map> deleteVectorAndGetUpdatedIds(labelType label) override; -#ifdef BUILD_TESTS void getDataByLabel(labelType label, std::vector> &vectors_output) const override { - auto ids = labelToIdsLookup.find(label); + if (ids == labelToIdsLookup.end()) { + return; + } for (idType id : ids->second) { auto vec = std::vector(this->dim); @@ -54,6 +55,8 @@ class BruteForceIndex_Multi : public BruteForceIndex { } } +#ifdef BUILD_TESTS + std::vector> getStoredVectorDataByLabel(labelType label) const override { std::vector> vectors_output; auto ids = labelToIdsLookup.find(label); diff --git a/src/VecSim/algorithms/brute_force/brute_force_single.h b/src/VecSim/algorithms/brute_force/brute_force_single.h index 9afe46ed3..c9155483a 100644 --- a/src/VecSim/algorithms/brute_force/brute_force_single.h +++ b/src/VecSim/algorithms/brute_force/brute_force_single.h @@ -42,19 +42,22 @@ class BruteForceIndex_Single : public BruteForceIndex { // We call this when we KNOW that the label exists in the index. idType getIdOfLabel(labelType label) const { return labelToIdLookup.find(label)->second; } -#ifdef BUILD_TESTS void getDataByLabel(labelType label, std::vector> &vectors_output) const override { - - auto id = labelToIdLookup.at(label); + auto it = labelToIdLookup.find(label); + if (it == labelToIdLookup.end()) { + return; + } auto vec = std::vector(this->dim); // Only copy the vector data (dim * sizeof(DataType)), not any additional metadata like the // norm - memcpy(vec.data(), this->getDataByInternalId(id), this->dim * sizeof(DataType)); + memcpy(vec.data(), this->getDataByInternalId(it->second), this->dim * sizeof(DataType)); vectors_output.push_back(vec); } +#ifdef BUILD_TESTS + std::vector> getStoredVectorDataByLabel(labelType label) const override { std::vector> vectors_output; auto id = labelToIdLookup.at(label); diff --git a/src/VecSim/algorithms/hnsw/hnsw_multi.h b/src/VecSim/algorithms/hnsw/hnsw_multi.h index 736045ccd..bd5e15ca3 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_multi.h +++ b/src/VecSim/algorithms/hnsw/hnsw_multi.h @@ -68,10 +68,13 @@ class HNSWIndex_Multi : public HNSWIndex { : HNSWIndex(input, params, abstractInitParams, components, version), labelLookup(this->maxElements, this->allocator) {} +#endif void getDataByLabel(labelType label, std::vector> &vectors_output) const override { - auto ids = labelLookup.find(label); + if (ids == labelLookup.end()) { + return; + } for (idType id : ids->second) { auto vec = std::vector(this->dim); @@ -82,6 +85,7 @@ class HNSWIndex_Multi : public HNSWIndex { } } +#ifdef BUILD_TESTS std::vector> getStoredVectorDataByLabel(labelType label) const override { std::vector> vectors_output; auto ids = labelLookup.find(label); diff --git a/src/VecSim/algorithms/hnsw/hnsw_single.h b/src/VecSim/algorithms/hnsw/hnsw_single.h index 99fcf7652..df62fb3e3 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_single.h +++ b/src/VecSim/algorithms/hnsw/hnsw_single.h @@ -44,18 +44,23 @@ class HNSWIndex_Single : public HNSWIndex { : HNSWIndex(input, params, abstractInitParams, components, version), labelLookup(this->maxElements, this->allocator) {} +#endif void getDataByLabel(labelType label, std::vector> &vectors_output) const override { - - auto id = labelLookup.at(label); + auto it = labelLookup.find(label); + if (it == labelLookup.end()) { + return; + } auto vec = std::vector(this->dim); // Only copy the vector data (dim * sizeof(DataType)), not any additional metadata like the // norm - memcpy(vec.data(), this->getDataByInternalId(id), this->dim * sizeof(DataType)); + memcpy(vec.data(), this->getDataByInternalId(it->second), this->dim * sizeof(DataType)); vectors_output.push_back(vec); } +#ifdef BUILD_TESTS + std::vector> getStoredVectorDataByLabel(labelType label) const override { std::vector> vectors_output; auto id = labelLookup.at(label); diff --git a/src/VecSim/vec_sim_index.h b/src/VecSim/vec_sim_index.h index 99fc002a1..706167335 100644 --- a/src/VecSim/vec_sim_index.h +++ b/src/VecSim/vec_sim_index.h @@ -327,6 +327,26 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { }; return info; } + /** + * @brief Get the vector elements stored under a given label, in insertion order. + * + * Appends nothing when the label is absent, so the output size doubles as the answer to + * whether the index holds it. + * + * Returns ONLY the vector elements, even where the stored vector carries extra + * metadata -- with int8_t/uint8_t under cosine, the trailing norm is not included. + * Use getStoredVectorDataByLabel() when the complete stored form is wanted. + * + * The elements are the *stored* ones, i.e. after any insert-time preprocessing: + * normalized under cosine, and for a quantized index whatever that index can + * reconstruct rather than the blob originally handed to addVector. + * + * @param label The label to retrieve vector(s) elements for + * @param vectors_output Empty vector to be filled with vector(s) + */ + virtual void getDataByLabel(labelType label, + std::vector> &vectors_output) const {} + #ifdef BUILD_TESTS void replacePPContainer(PreprocessorsContainerAbstract *newPPContainer) { delete this->preprocessors; @@ -341,24 +361,6 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { // without requiring an actual disk-based index implementation. void setIsDiskForTesting(bool v) { this->isDisk = v; } - /** - * @brief Used for testing - get only the vector elements associated with a given label. - * This function copies only the vector(s) elements into the output vector, - * without any additional metadata that might be stored with the vector. - * - * Important: This method returns ONLY the vector elements, even if the stored vector contains - * additional metadata. For example, with int8_t/uint8_t vectors using cosine similarity, - * this method will NOT return the norm that is stored with the vector(s). - * - * If you need the complete data including any metadata, use getStoredVectorDataByLabel() - * instead. - * - * @param label The label to retrieve vector(s) elements for - * @param vectors_output Empty vector to be filled with vector(s) - */ - virtual void getDataByLabel(labelType label, - std::vector> &vectors_output) const = 0; - /** * @brief Used for testing - get the complete raw data associated with a given label. * This function returns the ENTIRE vector(s) data as stored in the index, including any From 8abee6d4e2994e682039f987de578cc102f9383d Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Wed, 26 Aug 2026 14:18:17 +0300 Subject: [PATCH 02/13] Read a tiered index's label from the tier holding it `TieredHNSWIndex::getDataByLabel` delegated to the backend index alone, so a label still sitting in the flat buffer -- everything written since the last ingest job -- reported nothing. That is the wrong answer for a caller asking what a label holds, and it is worst for recently written vectors, which are the ones most likely to be written again. The lookup asks the frontend first and falls back to the backend, under the guards `relabelVector` takes and in the same order. Nothing about it is HNSW-specific -- both tiers are plain indexes -- so it lives on `VecSimTieredIndex`, where the SVS tiered index gets it too, and the HNSW-specific override is gone. Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/algorithms/hnsw/hnsw_tiered.h | 9 +-------- src/VecSim/vec_sim_tiered_index.h | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index b90ba8e69..5dff5c931 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -259,8 +259,8 @@ class TieredHNSWIndex : public VecSimTieredIndex { return res; } + #ifdef BUILD_TESTS - void getDataByLabel(labelType label, std::vector> &vectors_output) const; size_t indexMetaDataCapacity() const override { return this->backendIndex->indexMetaDataCapacity() + this->frontendIndex->indexMetaDataCapacity(); @@ -1248,11 +1248,4 @@ VecSimIndexBasicInfo TieredHNSWIndex::basicInfo() const { return info; } -#ifdef BUILD_TESTS -template -void TieredHNSWIndex::getDataByLabel( - labelType label, std::vector> &vectors_output) const { - this->getHNSWIndex()->getDataByLabel(label, vectors_output); -} -#endif diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index d097b1ae4..27ac82595 100644 --- a/src/VecSim/vec_sim_tiered_index.h +++ b/src/VecSim/vec_sim_tiered_index.h @@ -115,6 +115,29 @@ class VecSimTieredIndex : public VecSimIndexInterface { VecSimQueryReply_Order order) const; public: + /** + * @brief Get the vector elements stored under a label, in insertion order. + * + * Contract on `VecSimIndexAbstract::getDataByLabel`. A label lives in the flat buffer until + * an ingest job moves it to the backend, so both tiers are asked -- reading only the backend + * would report nothing for every vector written recently enough to still be buffered, which + * is exactly when a document is most likely to be updated again. The guards are the ones + * `relabelVector` takes, in the same order. + */ + void getDataByLabel(labelType label, std::vector> &vectors_output) const { + this->flatIndexGuard.lock_shared(); + if (this->frontendIndex->isLabelExists(label)) { + this->frontendIndex->getDataByLabel(label, vectors_output); + this->flatIndexGuard.unlock_shared(); + return; + } + this->flatIndexGuard.unlock_shared(); + + this->mainIndexGuard.lock_shared(); + this->backendIndex->getDataByLabel(label, vectors_output); + this->mainIndexGuard.unlock_shared(); + } + VecSimTieredIndex(VecSimIndexAbstract *backendIndex_, BruteForceIndex *frontendIndex_, TieredIndexParams tieredParams, std::shared_ptr allocator) From e41b64d8090223ddea5cb5d81f5656a05095afe7 Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Wed, 26 Aug 2026 14:44:06 +0300 Subject: [PATCH 03/13] Guard the label lookup and data blocks in getDataByLabel The accessor read `labelLookup` and the data blocks with no lock, which was tolerable while it was test-only and single-threaded, and is not now that production code can call it. A shared `mainIndexGuard` is not enough to make the read safe. Tiered ingest takes exactly that lock plus `indexDataGuard` and then stores a new element -- rehashing the map and possibly resizing the data blocks -- and `markDelete` mutates under the same inner guard. So a reader holding only the main lock can observe a rehash in progress or a block that has moved. The guard belongs to the accessor rather than its callers, matching `getLabelsSet`: the tiered lookup added in the previous commit holds the tier-selection guards and lets each tier's accessor take its own, which is the division `computeUnifiedIndexLabelsSetUnsafe` already relies on. Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/algorithms/hnsw/hnsw_multi.h | 4 ++++ src/VecSim/algorithms/hnsw/hnsw_single.h | 4 ++++ src/VecSim/vec_sim_tiered_index.h | 10 ++++++++-- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw_multi.h b/src/VecSim/algorithms/hnsw/hnsw_multi.h index bd5e15ca3..a38e88a5b 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_multi.h +++ b/src/VecSim/algorithms/hnsw/hnsw_multi.h @@ -71,6 +71,10 @@ class HNSWIndex_Multi : public HNSWIndex { #endif void getDataByLabel(labelType label, std::vector> &vectors_output) const override { + // See the single-value implementation: the guard is the accessor's to take, because + // `indexDataGuard` -- not the main lock -- is what ingest and `markDelete` hold while + // mutating `labelLookup` and the data blocks. + std::shared_lock index_data_lock(this->indexDataGuard); auto ids = labelLookup.find(label); if (ids == labelLookup.end()) { return; diff --git a/src/VecSim/algorithms/hnsw/hnsw_single.h b/src/VecSim/algorithms/hnsw/hnsw_single.h index df62fb3e3..897af7b85 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_single.h +++ b/src/VecSim/algorithms/hnsw/hnsw_single.h @@ -47,6 +47,10 @@ class HNSWIndex_Single : public HNSWIndex { #endif void getDataByLabel(labelType label, std::vector> &vectors_output) const override { + // `labelLookup` and the data blocks are mutated under `indexDataGuard` by callers that + // hold no more than a shared main lock -- tiered ingest and `markDelete` both do -- so + // reading them needs this guard, not the caller's. Same reason `getLabelsSet` takes it. + std::shared_lock index_data_lock(this->indexDataGuard); auto it = labelLookup.find(label); if (it == labelLookup.end()) { return; diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index 27ac82595..bc0a1535c 100644 --- a/src/VecSim/vec_sim_tiered_index.h +++ b/src/VecSim/vec_sim_tiered_index.h @@ -121,8 +121,14 @@ class VecSimTieredIndex : public VecSimIndexInterface { * Contract on `VecSimIndexAbstract::getDataByLabel`. A label lives in the flat buffer until * an ingest job moves it to the backend, so both tiers are asked -- reading only the backend * would report nothing for every vector written recently enough to still be buffered, which - * is exactly when a document is most likely to be updated again. The guards are the ones - * `relabelVector` takes, in the same order. + * is exactly when a document is most likely to be updated again. + * + * The guards taken here are the ones `relabelVector` takes, in the same order, and they cover + * tier selection and the frontend read. They are deliberately not the whole story: the + * backend's own data guard is taken by its `getDataByLabel`, because a shared main lock does + * not exclude an ingest that mutates under `indexDataGuard`. Same division as + * `computeUnifiedIndexLabelsSetUnsafe`, which holds the outer locks and lets `getLabelsSet` + * take the inner one. */ void getDataByLabel(labelType label, std::vector> &vectors_output) const { this->flatIndexGuard.lock_shared(); From d012e58fde6664c41d9ddc323e91588e54ace837 Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Wed, 26 Aug 2026 14:54:00 +0300 Subject: [PATCH 04/13] Read both tiers for a multi-value label Returning as soon as the flat buffer held the label was right only for a single-value index. In a multi-value one a label's vectors are routinely split across the tiers while an ingest job is pending, so the buffer alone is a subset and the already-ingested vectors were missing from the result. Which tiers to read now follows `getDistanceFrom_Unsafe`, which faced the same choice: short-circuit on a buffer hit only when the index is single-value, otherwise read the backend as well. Two properties a tiered read cannot avoid are documented rather than papered over. The vectors come out buffer-first, which for a split label is not insertion order; and an ingest job inserts into the backend before removing from the buffer, so a vector caught inside that window is reported by both tiers. `flatIndexGuard` is held across both reads -- it cannot prevent the duplicate, but it does stop the buffer's copy disappearing between them, which is what would turn a duplicate into an omission. The test covers the split, the ingested state, and an absent label. It fails on the previous implementation with the subset it returned. Co-Authored-By: Claude Opus 5 (1M context) --- .../hnsw/hnsw_tiered_tests_friends.h | 1 + src/VecSim/vec_sim_index.h | 3 ++ src/VecSim/vec_sim_tiered_index.h | 39 +++++++++------- tests/unit/test_hnsw_tiered.cpp | 45 +++++++++++++++++++ 4 files changed, 72 insertions(+), 16 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h b/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h index d4d5cd999..e2646060a 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h @@ -42,6 +42,7 @@ INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTest_RangeSearch_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTest_parallelRangeSearch_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_insertJobAsync_Test) +INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_getDataByLabelSpansBothTiers_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_insertJobAsyncMulti_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_KNNSearch_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_MergeMulti_Test) diff --git a/src/VecSim/vec_sim_index.h b/src/VecSim/vec_sim_index.h index 706167335..0bbc9542d 100644 --- a/src/VecSim/vec_sim_index.h +++ b/src/VecSim/vec_sim_index.h @@ -330,6 +330,9 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { /** * @brief Get the vector elements stored under a given label, in insertion order. * + * A tiered index cannot honour the ordering across its tiers; see + * `VecSimTieredIndex::getDataByLabel`. + * * Appends nothing when the label is absent, so the output size doubles as the answer to * whether the index holds it. * diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index bc0a1535c..aafe0cca7 100644 --- a/src/VecSim/vec_sim_tiered_index.h +++ b/src/VecSim/vec_sim_tiered_index.h @@ -118,30 +118,37 @@ class VecSimTieredIndex : public VecSimIndexInterface { /** * @brief Get the vector elements stored under a label, in insertion order. * - * Contract on `VecSimIndexAbstract::getDataByLabel`. A label lives in the flat buffer until - * an ingest job moves it to the backend, so both tiers are asked -- reading only the backend - * would report nothing for every vector written recently enough to still be buffered, which - * is exactly when a document is most likely to be updated again. + * Contract on `VecSimIndexAbstract::getDataByLabel`, with two caveats a tiered index cannot + * avoid, both of which only ever make an equality-testing caller answer "different": * - * The guards taken here are the ones `relabelVector` takes, in the same order, and they cover - * tier selection and the frontend read. They are deliberately not the whole story: the - * backend's own data guard is taken by its `getDataByLabel`, because a shared main lock does - * not exclude an ingest that mutates under `indexDataGuard`. Same division as + * - 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 + * caught inside that window is reported by both tiers and appears twice. + * + * Which tiers are read follows `getDistanceFrom_Unsafe`: a single-value label found in the + * buffer is the whole answer, but a multi-value label's vectors are routinely split across + * the tiers while an ingest is pending, so there the backend is read as well. Reading only + * 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. */ void getDataByLabel(labelType label, std::vector> &vectors_output) const { this->flatIndexGuard.lock_shared(); - if (this->frontendIndex->isLabelExists(label)) { - this->frontendIndex->getDataByLabel(label, vectors_output); - this->flatIndexGuard.unlock_shared(); - return; + this->frontendIndex->getDataByLabel(label, vectors_output); + if (this->backendIndex->isMultiValue() || vectors_output.empty()) { + this->mainIndexGuard.lock_shared(); + this->backendIndex->getDataByLabel(label, vectors_output); + this->mainIndexGuard.unlock_shared(); } this->flatIndexGuard.unlock_shared(); - - this->mainIndexGuard.lock_shared(); - this->backendIndex->getDataByLabel(label, vectors_output); - this->mainIndexGuard.unlock_shared(); } VecSimTieredIndex(VecSimIndexAbstract *backendIndex_, diff --git a/tests/unit/test_hnsw_tiered.cpp b/tests/unit/test_hnsw_tiered.cpp index a0927790b..647fb213e 100644 --- a/tests/unit/test_hnsw_tiered.cpp +++ b/tests/unit/test_hnsw_tiered.cpp @@ -486,6 +486,51 @@ TYPED_TEST(HNSWTieredIndexTestBasic, insertJobAsync) { } } +// A multi-value label's vectors are routinely split across the tiers while an ingest job is +// pending, so reading one tier reports a subset. Nothing else covers the tiered wrapper's +// `getDataByLabel` -- the other uses in this file all call a tier directly. +TYPED_TEST(HNSWTieredIndexTestBasic, getDataByLabelSpansBothTiers) { + size_t dim = 4; + HNSWParams params = { + .type = TypeParam::get_index_type(), .dim = dim, .metric = VecSimMetric_L2, .multi = true}; + VecSimParams hnsw_params = CreateParams(params); + auto mock_thread_pool = tieredIndexMock(); + auto *tiered_index = this->CreateTieredHNSWIndex(hnsw_params, mock_thread_pool); + auto allocator = tiered_index->getAllocator(); + + TEST_DATA_T first[dim]; + TEST_DATA_T second[dim]; + GenerateVector(first, dim, 1); + GenerateVector(second, dim, 2); + + // Two vectors under one label, with only the first ingested: one vector per tier. + VecSimIndex_AddVector(tiered_index, first, 0); + mock_thread_pool.thread_iteration(); + VecSimIndex_AddVector(tiered_index, second, 0); + ASSERT_EQ(tiered_index->backendIndex->indexSize(), 1); + ASSERT_EQ(tiered_index->frontendIndex->indexSize(), 1); + + std::vector> stored; + tiered_index->getDataByLabel(0, stored); + ASSERT_EQ(stored.size(), 2) << "a tier was not read; the label's vectors are split across them"; + // Buffer first, then backend: `second` is the buffered one. + ASSERT_NO_FATAL_FAILURE(CompareVectors(stored[0].data(), second, dim)); + ASSERT_NO_FATAL_FAILURE(CompareVectors(stored[1].data(), first, dim)); + + // Once everything is ingested the backend alone holds both. + mock_thread_pool.thread_iteration(); + ASSERT_EQ(tiered_index->frontendIndex->indexSize(), 0); + stored.clear(); + tiered_index->getDataByLabel(0, stored); + ASSERT_EQ(stored.size(), 2); + + // An absent label yields nothing rather than throwing, which is what lets the output size + // stand in for "does the index hold this label". + stored.clear(); + tiered_index->getDataByLabel(12345, stored); + ASSERT_TRUE(stored.empty()); +} + TYPED_TEST(HNSWTieredIndexTestBasic, insertJobAsyncMulti) { // Create TieredHNSW index instance with a mock queue. size_t dim = 4; From 09683d83d95cf9487f98af29272e22a366318fc1 Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Wed, 26 Aug 2026 17:09:28 +0300 Subject: [PATCH 05/13] Report nothing for quantized storage, and give SVS its own answer Two ways the accessor could answer with values that are not there. A quantized element is smaller than its elements would be -- SQ8 keeps one byte per dimension plus a few metadata floats, against `dim * sizeof(DataType)` -- and vectors are packed back-to-back at `storedDataSize`. Copying the elements size therefore ran off the end of the element, into its neighbour or past the block for the last one, and reinterpreted the compression as floats. Nothing here dequantizes, so the size check now reports nothing instead, which the contract already defines as "cannot tell". The docstring claiming a reconstruction is corrected: there never was one. `SVSIndex` kept its `getDataByLabel` in a test-only block, asserting "Not implemented", and relied on the base's default outside it. A tiered SVS index reaches it through `VecSimTieredIndex::getDataByLabel` as soon as a vector is ingested, so that assert was reachable from a test build. It now has one unconditional definition that appends nothing, which is its honest answer -- SVS keeps vectors in the SVS library's reduced form and does not hand them back -- and with every concrete class defining the method the base goes back to pure virtual, so a future index type has to state its answer rather than inherit silence. Tests cover the quantized refusal and reading an absent label on both single-value implementations, where `.at()` used to throw. Co-Authored-By: Claude Opus 5 (1M context) --- .../brute_force/brute_force_multi.h | 7 ++++++ .../brute_force/brute_force_single.h | 7 ++++++ src/VecSim/algorithms/hnsw/hnsw_multi.h | 7 ++++++ src/VecSim/algorithms/hnsw/hnsw_single.h | 7 ++++++ src/VecSim/algorithms/svs/svs.h | 20 ++++++++++++----- src/VecSim/vec_sim_index.h | 15 ++++++++----- tests/unit/test_bruteforce.cpp | 22 +++++++++++++++++++ tests/unit/test_hnsw.cpp | 20 +++++++++++++++++ tests/unit/test_hnsw_sq8.cpp | 19 ++++++++++++++++ 9 files changed, 113 insertions(+), 11 deletions(-) diff --git a/src/VecSim/algorithms/brute_force/brute_force_multi.h b/src/VecSim/algorithms/brute_force/brute_force_multi.h index 6bc9a668b..5f5173985 100644 --- a/src/VecSim/algorithms/brute_force/brute_force_multi.h +++ b/src/VecSim/algorithms/brute_force/brute_force_multi.h @@ -41,6 +41,13 @@ class BruteForceIndex_Multi : public BruteForceIndex { deleteVectorAndGetUpdatedIds(labelType label) override; void getDataByLabel(labelType label, std::vector> &vectors_output) const override { + // A quantized index stores fewer bytes than the elements occupy (SQ8 keeps one byte per + // dimension plus metadata), so copying `dim * sizeof(DataType)` would read past the + // element and reinterpret the compression as values. Nothing here dequantizes, so the + // honest answer is none: per the contract an empty output reads as "cannot tell". + if (this->getStoredDataSize() < this->dim * sizeof(DataType)) { + return; + } auto ids = labelToIdsLookup.find(label); if (ids == labelToIdsLookup.end()) { return; diff --git a/src/VecSim/algorithms/brute_force/brute_force_single.h b/src/VecSim/algorithms/brute_force/brute_force_single.h index c9155483a..87093f489 100644 --- a/src/VecSim/algorithms/brute_force/brute_force_single.h +++ b/src/VecSim/algorithms/brute_force/brute_force_single.h @@ -44,6 +44,13 @@ class BruteForceIndex_Single : public BruteForceIndex { void getDataByLabel(labelType label, std::vector> &vectors_output) const override { + // A quantized index stores fewer bytes than the elements occupy (SQ8 keeps one byte per + // dimension plus metadata), so copying `dim * sizeof(DataType)` would read past the + // element and reinterpret the compression as values. Nothing here dequantizes, so the + // honest answer is none: per the contract an empty output reads as "cannot tell". + if (this->getStoredDataSize() < this->dim * sizeof(DataType)) { + return; + } auto it = labelToIdLookup.find(label); if (it == labelToIdLookup.end()) { return; diff --git a/src/VecSim/algorithms/hnsw/hnsw_multi.h b/src/VecSim/algorithms/hnsw/hnsw_multi.h index a38e88a5b..e5d5f518b 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_multi.h +++ b/src/VecSim/algorithms/hnsw/hnsw_multi.h @@ -74,6 +74,13 @@ class HNSWIndex_Multi : public HNSWIndex { // See the single-value implementation: the guard is the accessor's to take, because // `indexDataGuard` -- not the main lock -- is what ingest and `markDelete` hold while // mutating `labelLookup` and the data blocks. + // A quantized index stores fewer bytes than the elements occupy (SQ8 keeps one byte per + // dimension plus metadata), so copying `dim * sizeof(DataType)` would read past the + // element and reinterpret the compression as values. Nothing here dequantizes, so the + // honest answer is none: per the contract an empty output reads as "cannot tell". + if (this->getStoredDataSize() < this->dim * sizeof(DataType)) { + return; + } std::shared_lock index_data_lock(this->indexDataGuard); auto ids = labelLookup.find(label); if (ids == labelLookup.end()) { diff --git a/src/VecSim/algorithms/hnsw/hnsw_single.h b/src/VecSim/algorithms/hnsw/hnsw_single.h index 897af7b85..7207d0585 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_single.h +++ b/src/VecSim/algorithms/hnsw/hnsw_single.h @@ -50,6 +50,13 @@ class HNSWIndex_Single : public HNSWIndex { // `labelLookup` and the data blocks are mutated under `indexDataGuard` by callers that // hold no more than a shared main lock -- tiered ingest and `markDelete` both do -- so // reading them needs this guard, not the caller's. Same reason `getLabelsSet` takes it. + // A quantized index stores fewer bytes than the elements occupy (SQ8 keeps one byte per + // dimension plus metadata), so copying `dim * sizeof(DataType)` would read past the + // element and reinterpret the compression as values. Nothing here dequantizes, so the + // honest answer is none: per the contract an empty output reads as "cannot tell". + if (this->getStoredDataSize() < this->dim * sizeof(DataType)) { + return; + } std::shared_lock index_data_lock(this->indexDataGuard); auto it = labelLookup.find(label); if (it == labelLookup.end()) { diff --git a/src/VecSim/algorithms/svs/svs.h b/src/VecSim/algorithms/svs/svs.h index 7dc15dc0d..8712feda6 100644 --- a/src/VecSim/algorithms/svs/svs.h +++ b/src/VecSim/algorithms/svs/svs.h @@ -799,14 +799,22 @@ class SVSIndex : public VecSimIndexAbstract, fl return vectors_output; } } - void getDataByLabel( - labelType label, - std::vector>> &vectors_output) const override { - assert(false && "Not implemented"); - } - svs::logging::logger_ptr getLogger() const override { return logger_; } #endif + + /** + * SVS keeps vectors in the SVS library's own form -- quantized, and for LeanVec reduced -- + * and does not hand them back, so this appends nothing. Per the contract on + * `VecSimIndexAbstract::getDataByLabel` an empty output reads as "cannot tell", which is + * the answer a caller needs; a comparison against what this index stores is not available. + * + * Defined unconditionally, and not left to a default: a tiered SVS index reaches this + * through `VecSimTieredIndex::getDataByLabel` as soon as a vector has been ingested, so a + * definition that exists only in test builds is a definition that aborts exactly there. + */ + void getDataByLabel( + labelType label, + std::vector>> &vectors_output) const override {} }; #ifdef BUILD_TESTS diff --git a/src/VecSim/vec_sim_index.h b/src/VecSim/vec_sim_index.h index 0bbc9542d..2df75209a 100644 --- a/src/VecSim/vec_sim_index.h +++ b/src/VecSim/vec_sim_index.h @@ -334,21 +334,26 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { * `VecSimTieredIndex::getDataByLabel`. * * Appends nothing when the label is absent, so the output size doubles as the answer to - * whether the index holds it. + * whether the index holds it -- and likewise for an index type that cannot read its stored + * vectors back at all, which is how `SVSIndex` answers. * * Returns ONLY the vector elements, even where the stored vector carries extra * metadata -- with int8_t/uint8_t under cosine, the trailing norm is not included. * Use getStoredVectorDataByLabel() when the complete stored form is wanted. * - * The elements are the *stored* ones, i.e. after any insert-time preprocessing: - * normalized under cosine, and for a quantized index whatever that index can - * reconstruct rather than the blob originally handed to addVector. + * The elements are the *stored* ones, i.e. after any insert-time preprocessing — + * normalized under cosine, for instance — and not the blob originally handed to + * addVector. + * + * Nothing here dequantizes. An index whose stored form is smaller than its elements (SQ8 + * keeps one byte per dimension plus metadata) therefore appends nothing rather than + * reinterpreting the compression as values, which reads as "cannot tell". * * @param label The label to retrieve vector(s) elements for * @param vectors_output Empty vector to be filled with vector(s) */ virtual void getDataByLabel(labelType label, - std::vector> &vectors_output) const {} + std::vector> &vectors_output) const = 0; #ifdef BUILD_TESTS void replacePPContainer(PreprocessorsContainerAbstract *newPPContainer) { diff --git a/tests/unit/test_bruteforce.cpp b/tests/unit/test_bruteforce.cpp index abf7e9855..351255d23 100644 --- a/tests/unit/test_bruteforce.cpp +++ b/tests/unit/test_bruteforce.cpp @@ -39,6 +39,28 @@ class BruteForceTest : public ::testing::Test { TYPED_TEST_SUITE(BruteForceTest, DataTypeSet); +// An absent label appends nothing rather than throwing. `labelToIdLookup.at()` used to be +// used here, so asking about a label the index does not hold was an exception; the contract +// now lets the output size answer whether it is held, which is what the tiered lookup and +// RediSearch's vector comparison both rely on. +TYPED_TEST(BruteForceTest, getDataByLabelAbsentLabel) { + size_t dim = 4; + BFParams params = {.dim = dim, .metric = VecSimMetric_L2}; + VecSimIndex *index = this->CreateNewIndex(params); + BruteForceIndex *bf_index = this->CastToBF(index); + + GenerateAndAddVector(index, dim, 1); + + std::vector> stored; + bf_index->getDataByLabel(2, stored); + ASSERT_TRUE(stored.empty()); + + bf_index->getDataByLabel(1, stored); + ASSERT_EQ(stored.size(), 1); + + VecSimIndex_Free(index); +} + TYPED_TEST(BruteForceTest, brute_force_vector_add_test) { size_t dim = 4; diff --git a/tests/unit/test_hnsw.cpp b/tests/unit/test_hnsw.cpp index dfd0e1083..7fddcecc6 100644 --- a/tests/unit/test_hnsw.cpp +++ b/tests/unit/test_hnsw.cpp @@ -89,6 +89,26 @@ TEST(HNSWDistanceDispatchTest, UsesStoredToQueryDispatch) { VecSimIndex_Free(index); } +// See the brute-force counterpart: an absent label appends nothing instead of throwing, which +// is what lets the output size stand in for "does the index hold this label". +TYPED_TEST(HNSWTest, getDataByLabelAbsentLabel) { + size_t dim = 4; + HNSWParams params = {.dim = dim, .metric = VecSimMetric_L2}; + VecSimIndex *index = this->CreateNewIndex(params); + HNSWIndex *hnsw_index = this->CastToHNSW(index); + + GenerateAndAddVector(index, dim, 1); + + std::vector> stored; + hnsw_index->getDataByLabel(2, stored); + ASSERT_TRUE(stored.empty()); + + hnsw_index->getDataByLabel(1, stored); + ASSERT_EQ(stored.size(), 1); + + VecSimIndex_Free(index); +} + TYPED_TEST(HNSWTest, hnsw_vector_add_test) { size_t dim = 4; diff --git a/tests/unit/test_hnsw_sq8.cpp b/tests/unit/test_hnsw_sq8.cpp index cb579d45c..dadee09e3 100644 --- a/tests/unit/test_hnsw_sq8.cpp +++ b/tests/unit/test_hnsw_sq8.cpp @@ -135,6 +135,25 @@ void HNSWSQ8Test::create_index_test() { EXPECT_EQ(index->basicInfo().algo, VecSimAlgo_HNSWLIB); } +// A quantized element is smaller than its elements would be -- one byte per dimension plus +// metadata, against `dim * sizeof(DataType)` -- so reading it as values would run past the +// element (into the next one, or past the block for the last) and reinterpret the compression +// as floats. `getDataByLabel` reports nothing instead, which callers read as "cannot tell". +TYPED_TEST(HNSWSQ8Test, getDataByLabelReportsNothingForQuantizedStorage) { + HNSWParams params = {.dim = 40, .M = 16, .efConstruction = 200}; + this->SetUp(params); + ASSERT_EQ(this->GenerateAndAddVector(0, 0.5f, 1.0f), 1); + + auto *hnsw_index = this->CastToHNSW(); + ASSERT_NE(hnsw_index, nullptr); + ASSERT_LT(hnsw_index->getStoredDataSize(), this->dim * sizeof(TEST_DATA_T)) + << "premise: SQ8 stores less than the elements occupy"; + + std::vector> stored; + hnsw_index->getDataByLabel(0, stored); + EXPECT_TRUE(stored.empty()); +} + TYPED_TEST(HNSWSQ8Test, CreateIndex) { this->create_index_test(); } TYPED_TEST(HNSWSQ8Test, RejectStandaloneCosine) { From 4ededcce38d41bda2d52877c532f9d2c4d9726bb Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Thu, 27 Aug 2026 09:13:28 +0300 Subject: [PATCH 06/13] Satisfy check-format, and cover the SVS accessor Three clang-format violations, all in hunks this branch introduced: two brace/blank-line spots left behind where `hnsw_tiered.h` lost its own `getDataByLabel`, and an empty body in `svs.h` that ran to column 101. The SVS override was also the branch's only uncovered code -- nothing called it. It is worth a test of its own rather than a coverage exemption: the behaviour it pins is that SVS reports nothing rather than pretending, and the alternative to defining it here was what let a tiered SVS index reach a not-implemented stub. Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/algorithms/hnsw/hnsw_tiered.h | 3 --- src/VecSim/algorithms/svs/svs.h | 3 ++- tests/unit/test_svs.cpp | 28 ++++++++++++++++++++++++ 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index 5dff5c931..1828774ad 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -259,7 +259,6 @@ class TieredHNSWIndex : public VecSimTieredIndex { return res; } - #ifdef BUILD_TESTS size_t indexMetaDataCapacity() const override { return this->backendIndex->indexMetaDataCapacity() + @@ -1247,5 +1246,3 @@ VecSimIndexBasicInfo TieredHNSWIndex::basicInfo() const { info.algo = VecSimAlgo_HNSWLIB; return info; } - - diff --git a/src/VecSim/algorithms/svs/svs.h b/src/VecSim/algorithms/svs/svs.h index 8712feda6..65a846d40 100644 --- a/src/VecSim/algorithms/svs/svs.h +++ b/src/VecSim/algorithms/svs/svs.h @@ -814,7 +814,8 @@ class SVSIndex : public VecSimIndexAbstract, fl */ void getDataByLabel( labelType label, - std::vector>> &vectors_output) const override {} + std::vector>> &vectors_output) const override { + } }; #ifdef BUILD_TESTS diff --git a/tests/unit/test_svs.cpp b/tests/unit/test_svs.cpp index 764f17316..bd7895f97 100644 --- a/tests/unit/test_svs.cpp +++ b/tests/unit/test_svs.cpp @@ -9,6 +9,7 @@ #include "gtest/gtest.h" #include "VecSim/vec_sim.h" +#include "VecSim/vec_sim_index.h" #include "unit_test_utils.h" #include #include @@ -2769,6 +2770,33 @@ 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. +// +// 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); + + GenerateAndAddVector(index, dim, 1); + ASSERT_EQ(VecSimIndex_IndexSize(index), 1); + + 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"; + + VecSimIndex_Free(index); +} + TEST(SVSTest, quant_modes) { // Limit VecSim log level to avoid printing too much information VecSimIndexInterface::setLogCallbackFunction(svsTestLogCallBackNoDebug); From 6f0ef7808c0dc6e6b14a56a8854448c2799fffef Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Thu, 27 Aug 2026 13:22:15 +0300 Subject: [PATCH 07/13] Identify quantized storage by asking the index, not by size The guard refusing to read a quantized element as values compared `getStoredDataSize()` against `dim * sizeof(DataType)`. That inequality only holds above a certain dimension: SQ8 stores one byte per dimension plus FP32 metadata, so at dim 4 with FP32/L2 the element is 20 bytes against 16 for the raw elements. Below the crossover the comparison concludes "not quantized" and hands back compression and metadata reinterpreted as floats, which is the outcome the guard exists to prevent. `VecSimIndexAbstract::isQuantized` is the reliable test, set by the factory alongside `storedDataSize` and already used for the same kind of branch in `HNSWIndex_Single::getDistanceFrom_Unsafe`. The test moves to dim 4, where the size comparison fails, and keeps a dim 40 case for the side it handled. The first version of this test used dim 40 only, which is why the bug survived it; the small-dimension case fails against the previous guard. Co-Authored-By: Claude Opus 5 (1M context) --- .../brute_force/brute_force_multi.h | 15 +++++--- .../brute_force/brute_force_single.h | 15 +++++--- src/VecSim/algorithms/hnsw/hnsw_multi.h | 15 +++++--- src/VecSim/algorithms/hnsw/hnsw_single.h | 15 +++++--- src/VecSim/vec_sim_index.h | 7 ++-- tests/unit/test_hnsw_sq8.cpp | 37 +++++++++++++++---- 6 files changed, 73 insertions(+), 31 deletions(-) diff --git a/src/VecSim/algorithms/brute_force/brute_force_multi.h b/src/VecSim/algorithms/brute_force/brute_force_multi.h index 9b4c4cee5..179445f55 100644 --- a/src/VecSim/algorithms/brute_force/brute_force_multi.h +++ b/src/VecSim/algorithms/brute_force/brute_force_multi.h @@ -42,11 +42,16 @@ class BruteForceIndex_Multi : public BruteForceIndex { deleteVectorAndGetUpdatedIds(labelType label) override; void getDataByLabel(labelType label, std::vector> &vectors_output) const override { - // A quantized index stores fewer bytes than the elements occupy (SQ8 keeps one byte per - // dimension plus metadata), so copying `dim * sizeof(DataType)` would read past the - // element and reinterpret the compression as values. Nothing here dequantizes, so the - // honest answer is none: per the contract an empty output reads as "cannot tell". - if (this->getStoredDataSize() < this->dim * sizeof(DataType)) { + // A quantized element is not the elements: SQ8 keeps one byte per dimension plus FP32 + // metadata, so copying `dim * sizeof(DataType)` out of it would reinterpret compression + // and metadata as values. Nothing here dequantizes, so the honest answer is none -- per + // the contract an empty output reads as "cannot tell". + // + // Asking the index rather than comparing sizes: the quantized element is only *smaller* + // than the raw elements above a certain dimension. At dim 4 with FP32/L2 it is larger + // (4 + 4*4 = 20 bytes against 16), so a size test concludes "not quantized" exactly where + // it matters most. + if (this->isQuantized) { return; } auto ids = labelToIdsLookup.find(label); diff --git a/src/VecSim/algorithms/brute_force/brute_force_single.h b/src/VecSim/algorithms/brute_force/brute_force_single.h index 39caf349f..9bbdb85cf 100644 --- a/src/VecSim/algorithms/brute_force/brute_force_single.h +++ b/src/VecSim/algorithms/brute_force/brute_force_single.h @@ -45,11 +45,16 @@ class BruteForceIndex_Single : public BruteForceIndex { void getDataByLabel(labelType label, std::vector> &vectors_output) const override { - // A quantized index stores fewer bytes than the elements occupy (SQ8 keeps one byte per - // dimension plus metadata), so copying `dim * sizeof(DataType)` would read past the - // element and reinterpret the compression as values. Nothing here dequantizes, so the - // honest answer is none: per the contract an empty output reads as "cannot tell". - if (this->getStoredDataSize() < this->dim * sizeof(DataType)) { + // A quantized element is not the elements: SQ8 keeps one byte per dimension plus FP32 + // metadata, so copying `dim * sizeof(DataType)` out of it would reinterpret compression + // and metadata as values. Nothing here dequantizes, so the honest answer is none -- per + // the contract an empty output reads as "cannot tell". + // + // Asking the index rather than comparing sizes: the quantized element is only *smaller* + // than the raw elements above a certain dimension. At dim 4 with FP32/L2 it is larger + // (4 + 4*4 = 20 bytes against 16), so a size test concludes "not quantized" exactly where + // it matters most. + if (this->isQuantized) { return; } auto it = labelToIdLookup.find(label); diff --git a/src/VecSim/algorithms/hnsw/hnsw_multi.h b/src/VecSim/algorithms/hnsw/hnsw_multi.h index 489eb6389..b5d764d01 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_multi.h +++ b/src/VecSim/algorithms/hnsw/hnsw_multi.h @@ -74,11 +74,16 @@ class HNSWIndex_Multi : public HNSWIndex { // See the single-value implementation: the guard is the accessor's to take, because // `indexDataGuard` -- not the main lock -- is what ingest and `markDelete` hold while // mutating `labelLookup` and the data blocks. - // A quantized index stores fewer bytes than the elements occupy (SQ8 keeps one byte per - // dimension plus metadata), so copying `dim * sizeof(DataType)` would read past the - // element and reinterpret the compression as values. Nothing here dequantizes, so the - // honest answer is none: per the contract an empty output reads as "cannot tell". - if (this->getStoredDataSize() < this->dim * sizeof(DataType)) { + // A quantized element is not the elements: SQ8 keeps one byte per dimension plus FP32 + // metadata, so copying `dim * sizeof(DataType)` out of it would reinterpret compression + // and metadata as values. Nothing here dequantizes, so the honest answer is none -- per + // the contract an empty output reads as "cannot tell". + // + // Asking the index rather than comparing sizes: the quantized element is only *smaller* + // than the raw elements above a certain dimension. At dim 4 with FP32/L2 it is larger + // (4 + 4*4 = 20 bytes against 16), so a size test concludes "not quantized" exactly where + // it matters most. + if (this->isQuantized) { return; } std::shared_lock index_data_lock(this->indexDataGuard); diff --git a/src/VecSim/algorithms/hnsw/hnsw_single.h b/src/VecSim/algorithms/hnsw/hnsw_single.h index b8b10b9a3..39a96fb8b 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_single.h +++ b/src/VecSim/algorithms/hnsw/hnsw_single.h @@ -50,11 +50,16 @@ class HNSWIndex_Single : public HNSWIndex { // `labelLookup` and the data blocks are mutated under `indexDataGuard` by callers that // hold no more than a shared main lock -- tiered ingest and `markDelete` both do -- so // reading them needs this guard, not the caller's. Same reason `getLabelsSet` takes it. - // A quantized index stores fewer bytes than the elements occupy (SQ8 keeps one byte per - // dimension plus metadata), so copying `dim * sizeof(DataType)` would read past the - // element and reinterpret the compression as values. Nothing here dequantizes, so the - // honest answer is none: per the contract an empty output reads as "cannot tell". - if (this->getStoredDataSize() < this->dim * sizeof(DataType)) { + // A quantized element is not the elements: SQ8 keeps one byte per dimension plus FP32 + // metadata, so copying `dim * sizeof(DataType)` out of it would reinterpret compression + // and metadata as values. Nothing here dequantizes, so the honest answer is none -- per + // the contract an empty output reads as "cannot tell". + // + // Asking the index rather than comparing sizes: the quantized element is only *smaller* + // than the raw elements above a certain dimension. At dim 4 with FP32/L2 it is larger + // (4 + 4*4 = 20 bytes against 16), so a size test concludes "not quantized" exactly where + // it matters most. + if (this->isQuantized) { return; } std::shared_lock index_data_lock(this->indexDataGuard); diff --git a/src/VecSim/vec_sim_index.h b/src/VecSim/vec_sim_index.h index 2df75209a..5633628e8 100644 --- a/src/VecSim/vec_sim_index.h +++ b/src/VecSim/vec_sim_index.h @@ -345,9 +345,10 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { * normalized under cosine, for instance — and not the blob originally handed to * addVector. * - * Nothing here dequantizes. An index whose stored form is smaller than its elements (SQ8 - * keeps one byte per dimension plus metadata) therefore appends nothing rather than - * reinterpreting the compression as values, which reads as "cannot tell". + * Nothing here dequantizes. A quantized index therefore appends nothing rather than + * reinterpreting compression and metadata as values, which reads as "cannot tell". The test + * is the index's own `isQuantized`, not a size comparison: an SQ8 element is smaller than the + * raw elements only above a certain dimension, and larger below it. * * @param label The label to retrieve vector(s) elements for * @param vectors_output Empty vector to be filled with vector(s) diff --git a/tests/unit/test_hnsw_sq8.cpp b/tests/unit/test_hnsw_sq8.cpp index dadee09e3..6f4c9b451 100644 --- a/tests/unit/test_hnsw_sq8.cpp +++ b/tests/unit/test_hnsw_sq8.cpp @@ -135,19 +135,40 @@ void HNSWSQ8Test::create_index_test() { EXPECT_EQ(index->basicInfo().algo, VecSimAlgo_HNSWLIB); } -// A quantized element is smaller than its elements would be -- one byte per dimension plus -// metadata, against `dim * sizeof(DataType)` -- so reading it as values would run past the -// element (into the next one, or past the block for the last) and reinterpret the compression -// as floats. `getDataByLabel` reports nothing instead, which callers read as "cannot tell". +// A quantized element is not the elements, so reading it as values would hand back compression +// and FP32 metadata reinterpreted as floats. `getDataByLabel` reports nothing instead, which +// callers read as "cannot tell". +// +// dim 4 on purpose. SQ8 stores one byte per dimension plus metadata -- 4 slots for L2 -- so the +// element is 20 bytes here against 16 for the raw elements: *larger*. The first version of this +// guard compared sizes and so concluded "not quantized" for exactly these dimensions, and the +// first version of this test used dim 40, where the comparison happens to hold. Both dimensions +// are covered below for that reason. TYPED_TEST(HNSWSQ8Test, getDataByLabelReportsNothingForQuantizedStorage) { - HNSWParams params = {.dim = 40, .M = 16, .efConstruction = 200}; - this->SetUp(params); + // Small dim: the quantized element is *larger* than the raw elements, which is what defeats a + // size-based test. + HNSWParams small = {.dim = 4, .M = 16, .efConstruction = 200}; + this->SetUp(small); ASSERT_EQ(this->GenerateAndAddVector(0, 0.5f, 1.0f), 1); + auto *hnsw_index = this->CastToHNSW(); + ASSERT_NE(hnsw_index, nullptr); + ASSERT_GT(hnsw_index->getStoredDataSize(), this->dim * sizeof(TEST_DATA_T)) + << "premise: at dim 4 the quantized element is larger than the raw elements, so a size " + "comparison cannot identify it"; + + std::vector> stored; + hnsw_index->getDataByLabel(0, stored); + EXPECT_TRUE(stored.empty()); +} +TYPED_TEST(HNSWSQ8Test, getDataByLabelReportsNothingForQuantizedStorageLargeDim) { + // Large dim: here the quantized element *is* smaller, the case the old size test handled. + HNSWParams large = {.dim = 40, .M = 16, .efConstruction = 200}; + this->SetUp(large); + ASSERT_EQ(this->GenerateAndAddVector(0, 0.5f, 1.0f), 1); auto *hnsw_index = this->CastToHNSW(); ASSERT_NE(hnsw_index, nullptr); - ASSERT_LT(hnsw_index->getStoredDataSize(), this->dim * sizeof(TEST_DATA_T)) - << "premise: SQ8 stores less than the elements occupy"; + ASSERT_LT(hnsw_index->getStoredDataSize(), this->dim * sizeof(TEST_DATA_T)); std::vector> stored; hnsw_index->getDataByLabel(0, stored); From 7e6cf9977c156ca1fd6fba4640c34bbe5afc668c Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Thu, 27 Aug 2026 13:41:14 +0300 Subject: [PATCH 08/13] Hold the tiered read's guards with shared_lock `getDataByLabel` took `flatIndexGuard` and `mainIndexGuard` with bare lock_shared / unlock_shared pairs, copying the style used elsewhere in the file. Both calls it wraps append to the caller's vector and so can throw `bad_alloc`, and on that path the manual unlock never runs -- leaving a reader guard held for the life of the process, which blocks every subsequent writer. Scoped guards also give the release order for free: main is released before flat, the reverse of the acquisition order the lock hierarchy requires. Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/vec_sim_tiered_index.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index aafe0cca7..f16c0f811 100644 --- a/src/VecSim/vec_sim_tiered_index.h +++ b/src/VecSim/vec_sim_tiered_index.h @@ -141,14 +141,12 @@ class VecSimTieredIndex : public VecSimIndexInterface { * take the inner one. */ void getDataByLabel(labelType label, std::vector> &vectors_output) const { - this->flatIndexGuard.lock_shared(); + std::shared_lock flat_lock(this->flatIndexGuard); this->frontendIndex->getDataByLabel(label, vectors_output); if (this->backendIndex->isMultiValue() || vectors_output.empty()) { - this->mainIndexGuard.lock_shared(); + std::shared_lock main_lock(this->mainIndexGuard); this->backendIndex->getDataByLabel(label, vectors_output); - this->mainIndexGuard.unlock_shared(); } - this->flatIndexGuard.unlock_shared(); } VecSimTieredIndex(VecSimIndexAbstract *backendIndex_, From f15ea14cc5e669c978dd09676393648522284750 Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Thu, 27 Aug 2026 15:10:23 +0300 Subject: [PATCH 09/13] Move the vector into the output instead of copying it `getDataByLabel` built each vector locally and then copied it into the caller's output -- a second allocation and memcpy per stored vector, on a path that is now production code rather than a test helper. The local is dead after the push, so it can be moved. `getStoredVectorDataByLabel` directly below each of these already moved; this brings the four `getDataByLabel` implementations in line with it. Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/algorithms/brute_force/brute_force_multi.h | 2 +- src/VecSim/algorithms/brute_force/brute_force_single.h | 2 +- src/VecSim/algorithms/hnsw/hnsw_multi.h | 2 +- src/VecSim/algorithms/hnsw/hnsw_single.h | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/VecSim/algorithms/brute_force/brute_force_multi.h b/src/VecSim/algorithms/brute_force/brute_force_multi.h index 179445f55..b14549d99 100644 --- a/src/VecSim/algorithms/brute_force/brute_force_multi.h +++ b/src/VecSim/algorithms/brute_force/brute_force_multi.h @@ -64,7 +64,7 @@ class BruteForceIndex_Multi : public BruteForceIndex { // Only copy the vector data (dim * sizeof(DataType)), not any additional metadata like // the norm memcpy(vec.data(), this->getDataByInternalId(id), this->dim * sizeof(DataType)); - vectors_output.push_back(vec); + vectors_output.push_back(std::move(vec)); } } diff --git a/src/VecSim/algorithms/brute_force/brute_force_single.h b/src/VecSim/algorithms/brute_force/brute_force_single.h index 9bbdb85cf..360a28374 100644 --- a/src/VecSim/algorithms/brute_force/brute_force_single.h +++ b/src/VecSim/algorithms/brute_force/brute_force_single.h @@ -66,7 +66,7 @@ class BruteForceIndex_Single : public BruteForceIndex { // Only copy the vector data (dim * sizeof(DataType)), not any additional metadata like the // norm memcpy(vec.data(), this->getDataByInternalId(it->second), this->dim * sizeof(DataType)); - vectors_output.push_back(vec); + vectors_output.push_back(std::move(vec)); } #ifdef BUILD_TESTS diff --git a/src/VecSim/algorithms/hnsw/hnsw_multi.h b/src/VecSim/algorithms/hnsw/hnsw_multi.h index b5d764d01..ef283fbf8 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_multi.h +++ b/src/VecSim/algorithms/hnsw/hnsw_multi.h @@ -97,7 +97,7 @@ class HNSWIndex_Multi : public HNSWIndex { // Only copy the vector data (dim * sizeof(DataType)), not any additional metadata like // the norm memcpy(vec.data(), this->getDataByInternalId(id), this->dim * sizeof(DataType)); - vectors_output.push_back(vec); + vectors_output.push_back(std::move(vec)); } } diff --git a/src/VecSim/algorithms/hnsw/hnsw_single.h b/src/VecSim/algorithms/hnsw/hnsw_single.h index 39a96fb8b..68cf236bc 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_single.h +++ b/src/VecSim/algorithms/hnsw/hnsw_single.h @@ -72,7 +72,7 @@ class HNSWIndex_Single : public HNSWIndex { // Only copy the vector data (dim * sizeof(DataType)), not any additional metadata like the // norm memcpy(vec.data(), this->getDataByInternalId(it->second), this->dim * sizeof(DataType)); - vectors_output.push_back(vec); + vectors_output.push_back(std::move(vec)); } #ifdef BUILD_TESTS From 9f0d52a968beeef562794d44de4e8437d9419a30 Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Thu, 27 Aug 2026 15:10:23 +0300 Subject: [PATCH 10/13] Do not consult a backend that cannot report stored vectors Two things the tiered read got wrong about its own contract and its backend. It assumed `vectors_output` arrives empty, and the assumption was load-bearing: for a single-value index a non-empty output made the "did the buffer have it" test false, so the backend was skipped and the label's vector was missed entirely. The tier decision now compares against a size snapshot instead of testing emptiness, so it holds whatever the caller passes, and a BUILD_TESTS assertion catches the caller that passes a reused vector and would otherwise silently get two labels' vectors concatenated. It also read the backend for an SVS index, which reports nothing by construction. Reaching that read means waiting on `mainIndexGuard` -- behind an SVS batch update, potentially for a while -- to be told so. The read is now skipped for an SVS backend, and SVS's own accessor logs at debug level rather than staying silent. Both are temporary: the type test is deliberately explicit rather than dressed up as architecture, and the note to remove it sits on `SVSIndex::getDataByLabel`, where whoever implements it will be working. Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/algorithms/svs/svs.h | 16 ++++++---------- src/VecSim/vec_sim_tiered_index.h | 29 +++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/src/VecSim/algorithms/svs/svs.h b/src/VecSim/algorithms/svs/svs.h index 65a846d40..c1c4babc0 100644 --- a/src/VecSim/algorithms/svs/svs.h +++ b/src/VecSim/algorithms/svs/svs.h @@ -802,19 +802,15 @@ class SVSIndex : public VecSimIndexAbstract, fl svs::logging::logger_ptr getLogger() const override { return logger_; } #endif - /** - * SVS keeps vectors in the SVS library's own form -- quantized, and for LeanVec reduced -- - * and does not hand them back, so this appends nothing. Per the contract on - * `VecSimIndexAbstract::getDataByLabel` an empty output reads as "cannot tell", which is - * the answer a caller needs; a comparison against what this index stores is not available. - * - * Defined unconditionally, and not left to a default: a tiered SVS index reaches this - * through `VecSimTieredIndex::getDataByLabel` as soon as a vector has been ingested, so a - * definition that exists only in test builds is a definition that aborts exactly there. - */ + // TODO: when this reports real data, remove the SVSIndexBase check in + // VecSimTieredIndex::getDataByLabel, which currently skips the backend read for SVS entirely. 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)); } }; diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index f16c0f811..fcfe95a13 100644 --- a/src/VecSim/vec_sim_tiered_index.h +++ b/src/VecSim/vec_sim_tiered_index.h @@ -18,6 +18,11 @@ #include +#if HAVE_SVS +// For the SVS special case in getDataByLabel; remove with it. +#include "VecSim/algorithms/svs/svs.h" +#endif + #define TIERED_LOG this->backendIndex->log /** @@ -118,7 +123,8 @@ class VecSimTieredIndex : public VecSimIndexInterface { /** * @brief Get the vector elements stored under a label, in insertion order. * - * Contract on `VecSimIndexAbstract::getDataByLabel`, with two caveats a tiered index cannot + * 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": * * - The vectors are the buffer's followed by the backend's, which for a multi-value label @@ -141,9 +147,28 @@ class VecSimTieredIndex : public VecSimIndexInterface { * take the inner one. */ void getDataByLabel(labelType label, std::vector> &vectors_output) const { +#ifdef BUILD_TESTS + // The base contract asks for an empty output. A caller reusing a vector would otherwise + // get this label's vectors appended to the previous label's, with nothing to notice it by. + assert(vectors_output.empty() && "getDataByLabel expects an empty output vector"); +#endif + + bool backend_can_report = true; +#if HAVE_SVS + // TODO: remove once SVSIndex::getDataByLabel reports real data. Until then the backend + // read below is guaranteed to append nothing, and reaching it means waiting on + // `mainIndexGuard` -- behind an SVS batch update, potentially for a while -- to be told + // so. The type test is deliberately explicit: it is a special case, not architecture. + backend_can_report = dynamic_cast(this->backendIndex) == nullptr; +#endif + std::shared_lock flat_lock(this->flatIndexGuard); + const size_t before_flat = vectors_output.size(); this->frontendIndex->getDataByLabel(label, vectors_output); - if (this->backendIndex->isMultiValue() || vectors_output.empty()) { + // 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 (backend_can_report && + (this->backendIndex->isMultiValue() || vectors_output.size() == before_flat)) { std::shared_lock main_lock(this->mainIndexGuard); this->backendIndex->getDataByLabel(label, vectors_output); } From 4a70839b4d3ccf65d343bf03f8aba65cc9f55e5d Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Thu, 27 Aug 2026 15:49:04 +0300 Subject: [PATCH 11/13] Say what implementing SVS getDataByLabel involves (MOD-17706) Both notes said what to delete without saying what to build. The one in svs.h now states what the method has to produce, why it is empty today, and the rule to carry over from the HNSW implementations: report nothing rather than a dequantized approximation, since a caller comparing stored bytes would read a reconstruction as a difference -- or as a match. It also records that implementing it does not by itself make an SVS-backed field relabel, because `relabelVector` is a separate gap. The tiered note lists what removal touches, and why the special case sits in the base class rather than as an override in `TieredSVSIndex`: the method is not virtual -- `VecSimIndexInterface` does not declare it -- and callers reach it through a `VecSimTieredIndex *`, so a derived override would never be found. Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/algorithms/svs/svs.h | 22 ++++++++++++++++++++-- src/VecSim/vec_sim_tiered_index.h | 21 ++++++++++++++++----- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/src/VecSim/algorithms/svs/svs.h b/src/VecSim/algorithms/svs/svs.h index c1c4babc0..ec15cabbd 100644 --- a/src/VecSim/algorithms/svs/svs.h +++ b/src/VecSim/algorithms/svs/svs.h @@ -802,8 +802,26 @@ class SVSIndex : public VecSimIndexAbstract, fl svs::logging::logger_ptr getLogger() const override { return logger_; } #endif - // TODO: when this reports real data, remove the SVSIndexBase check in - // VecSimTieredIndex::getDataByLabel, which currently skips the backend read for SVS entirely. + // 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. void getDataByLabel( labelType label, std::vector>> &vectors_output) const override { diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index fcfe95a13..adbd6e088 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. +// For the SVS special case in getDataByLabel; remove with it (MOD-17706). #include "VecSim/algorithms/svs/svs.h" #endif @@ -155,10 +155,21 @@ class VecSimTieredIndex : public VecSimIndexInterface { bool backend_can_report = true; #if HAVE_SVS - // TODO: remove once SVSIndex::getDataByLabel reports real data. Until then the backend - // read below is guaranteed to append nothing, and reaching it means waiting on - // `mainIndexGuard` -- behind an SVS batch update, potentially for a while -- to be told - // so. The type test is deliberately explicit: it is a special case, not architecture. + // TODO(MOD-17706): remove once SVSIndex::getDataByLabel reports real data. Removing it + // means deleting this block, the `backend_can_report` flag, and its term in the condition + // below, plus the guarded include of svs.h -- after which the plain condition does the + // right thing for every backend. + // + // Until then the backend read is guaranteed to append nothing, and reaching it means + // waiting on `mainIndexGuard` -- behind an SVS batch update, potentially for a while -- + // to be told so. + // + // 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; #endif From ac19831a90ad85a158b41d417f2b61d8814b8675 Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Thu, 27 Aug 2026 17:36:16 +0300 Subject: [PATCH 12/13] Skip the tiered read for a backend that cannot report Reading the tiers only to have the backend append nothing costs `mainIndexGuard`, which on a tiered index can queue behind a long exclusive holder. Worse, reading the buffer alone is a *partial* answer for a multi-value label split across the tiers, and a caller cannot tell a subset from the whole -- reporting nothing is the contract's "cannot tell", half of it is a wrong answer. Two backends cannot report. A quantized one stores compression plus metadata and nothing here dequantizes; that check is permanent. SVS has no per-label read of its stored vectors yet, and that check goes away with MOD-17706 -- kept separate from the quantized one so removing it cannot take the permanent check with it. `isQuantized` is protected on `VecSimIndexAbstract`, and `VecSimTieredIndex` derives from `VecSimIndexInterface`, so it needed a public getter alongside the existing `isMultiValue()`. Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/vec_sim_index.h | 1 + src/VecSim/vec_sim_tiered_index.h | 38 ++++++++++++++++--------------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/VecSim/vec_sim_index.h b/src/VecSim/vec_sim_index.h index 5633628e8..07833fe17 100644 --- a/src/VecSim/vec_sim_index.h +++ b/src/VecSim/vec_sim_index.h @@ -226,6 +226,7 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { inline size_t getDim() const { return dim; } inline void setLastSearchMode(VecSearchMode mode) override { this->lastMode = mode; } inline bool isMultiValue() const { return isMulti; } + inline bool isIndexQuantized() const { return isQuantized; } inline VecSimType getType() const { return vecType; } inline VecSimMetric getMetric() const { return metric; } inline size_t getStoredDataSize() const { return storedDataSize; } diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index adbd6e088..a53e2c18b 100644 --- a/src/VecSim/vec_sim_tiered_index.h +++ b/src/VecSim/vec_sim_tiered_index.h @@ -153,16 +153,17 @@ class VecSimTieredIndex : public VecSimIndexInterface { assert(vectors_output.empty() && "getDataByLabel expects an empty output vector"); #endif - bool backend_can_report = true; + // 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 = !this->backendIndex->isIndexQuantized(); #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 its term in the condition - // below, plus the guarded include of svs.h -- after which the plain condition does the - // right thing for every backend. + // means deleting this block and the guarded include of svs.h -- the flag above stays, for + // the quantized case. // - // Until then the backend read is guaranteed to append nothing, and reaching it means - // waiting on `mainIndexGuard` -- behind an SVS batch update, potentially for a while -- - // to be told so. + // Until then nothing is read at all for an SVS backend, for the same reason as a quantized + // one: the buffer alone would be a partial answer. 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 @@ -170,18 +171,19 @@ class VecSimTieredIndex : public VecSimIndexInterface { // 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; + backend_can_report = + backend_can_report && dynamic_cast(this->backendIndex) == nullptr; #endif - - 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 (backend_can_report && - (this->backendIndex->isMultiValue() || vectors_output.size() == before_flat)) { - std::shared_lock main_lock(this->mainIndexGuard); - this->backendIndex->getDataByLabel(label, vectors_output); + 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); + } } } From aaeafd0cfbe54834bfdb9ad9df130f636b0f2beb Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Thu, 27 Aug 2026 17:50:40 +0300 Subject: [PATCH 13/13] Cover the SVS tiered read, and drop the unreachable quantized check The tiered read had no test for either backend it skips. It has one now for SVS, with the vector deliberately left in the flat buffer -- the case where the frontend could have answered, which is what makes the test fail if the check or the no-fallback behaviour is undone. Checked against a build with the check disabled before trusting it. The quantized check goes away instead of gaining a test, because it cannot fire: `TieredFactory::NewIndex` returns nullptr for a quantized primary index and `EstimateInitialSize` throws, since the brute-force frontend is not quantized and the stored layouts would not match. A tiered index therefore never has a quantized backend. The guard that does the real work is the one inside the HNSW accessors, which covers the non-tiered path where quantization is reachable. `isIndexQuantized()` had no other caller and goes with it. Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/vec_sim_index.h | 1 - src/VecSim/vec_sim_tiered_index.h | 16 ++++++++-------- tests/unit/test_svs_tiered.cpp | 30 ++++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/VecSim/vec_sim_index.h b/src/VecSim/vec_sim_index.h index 07833fe17..5633628e8 100644 --- a/src/VecSim/vec_sim_index.h +++ b/src/VecSim/vec_sim_index.h @@ -226,7 +226,6 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { inline size_t getDim() const { return dim; } inline void setLastSearchMode(VecSearchMode mode) override { this->lastMode = mode; } inline bool isMultiValue() const { return isMulti; } - inline bool isIndexQuantized() const { return isQuantized; } inline VecSimType getType() const { return vecType; } inline VecSimMetric getMetric() const { return metric; } inline size_t getStoredDataSize() const { return storedDataSize; } diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index a53e2c18b..5b33f9bec 100644 --- a/src/VecSim/vec_sim_tiered_index.h +++ b/src/VecSim/vec_sim_tiered_index.h @@ -155,15 +155,16 @@ class VecSimTieredIndex : public VecSimIndexInterface { // 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 = !this->backendIndex->isIndexQuantized(); + bool backend_can_report = true; #if HAVE_SVS // TODO(MOD-17706): remove once SVSIndex::getDataByLabel reports real data. Removing it - // means deleting this block and the guarded include of svs.h -- the flag above stays, for - // the quantized case. + // 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, for the same reason as a quantized - // one: the buffer alone would be a partial answer. Skipping the reads also avoids waiting - // on `mainIndexGuard` behind an SVS batch update to be told nothing. + // 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 @@ -171,8 +172,7 @@ class VecSimTieredIndex : public VecSimIndexInterface { // 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 = - backend_can_report && dynamic_cast(this->backendIndex) == nullptr; + backend_can_report = dynamic_cast(this->backendIndex) == nullptr; #endif if (backend_can_report) { std::shared_lock flat_lock(this->flatIndexGuard); diff --git a/tests/unit/test_svs_tiered.cpp b/tests/unit/test_svs_tiered.cpp index 9542e7cd6..9a72787a2 100644 --- a/tests/unit/test_svs_tiered.cpp +++ b/tests/unit/test_svs_tiered.cpp @@ -487,6 +487,36 @@ 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) { + 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(); + + // Thresholds far above the single vector added below, so nothing is ingested and the label + // stays in the flat buffer. + auto *tiered_index = this->CreateTieredSVSIndex(svs_params, mock_thread_pool, 100, 100); + 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) + << "premise: the label is in the flat buffer, which could have reported it"; + + std::vector> stored; + tiered_index->getDataByLabel(0, stored); + EXPECT_TRUE(stored.empty()) + << "an SVS backend cannot report, so the tiered read reports nothing"; +} + TYPED_TEST(SVSTieredIndexTestBasic, ShrinkDuringScheduledUpdateIsDeferred) { // Regression for the original crash: shrink after the update job reserved threads, // but before the SVS update uses them. The update must still complete safely.