diff --git a/src/VecSim/algorithms/brute_force/brute_force_multi.h b/src/VecSim/algorithms/brute_force/brute_force_multi.h index 2b193e7ae..b14549d99 100644 --- a/src/VecSim/algorithms/brute_force/brute_force_multi.h +++ b/src/VecSim/algorithms/brute_force/brute_force_multi.h @@ -40,21 +40,36 @@ 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 { - + // 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); + if (ids == labelToIdsLookup.end()) { + return; + } for (idType id : ids->second) { 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)); - vectors_output.push_back(vec); + vectors_output.push_back(std::move(vec)); } } +#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 1519d7e85..360a28374 100644 --- a/src/VecSim/algorithms/brute_force/brute_force_single.h +++ b/src/VecSim/algorithms/brute_force/brute_force_single.h @@ -43,19 +43,34 @@ 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); + // 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); + 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)); - vectors_output.push_back(vec); + memcpy(vec.data(), this->getDataByInternalId(it->second), this->dim * sizeof(DataType)); + vectors_output.push_back(std::move(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 838556b0d..ef283fbf8 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_multi.h +++ b/src/VecSim/algorithms/hnsw/hnsw_multi.h @@ -68,20 +68,40 @@ 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 { - + // 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 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); auto ids = labelLookup.find(label); + if (ids == labelLookup.end()) { + return; + } for (idType id : ids->second) { 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)); - vectors_output.push_back(vec); + vectors_output.push_back(std::move(vec)); } } +#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 32e12c24a..68cf236bc 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_single.h +++ b/src/VecSim/algorithms/hnsw/hnsw_single.h @@ -44,18 +44,39 @@ 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); + // `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 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); + 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)); - vectors_output.push_back(vec); + memcpy(vec.data(), this->getDataByInternalId(it->second), this->dim * sizeof(DataType)); + vectors_output.push_back(std::move(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/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index 64a266906..11f6cf76f 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -261,7 +261,6 @@ class TieredHNSWIndex : public VecSimTieredIndex { } #ifdef BUILD_TESTS - void getDataByLabel(labelType label, std::vector> &vectors_output) const; size_t indexMetaDataCapacity() const override { return this->backendIndex->indexMetaDataCapacity() + this->frontendIndex->indexMetaDataCapacity(); @@ -1343,12 +1342,3 @@ VecSimIndexBasicInfo TieredHNSWIndex::basicInfo() const { info.algo = VecSimAlgo_HNSWLIB; 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/algorithms/hnsw/hnsw_tiered_tests_friends.h b/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h index 5e9316a0f..32a10cce5 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/algorithms/svs/svs.h b/src/VecSim/algorithms/svs/svs.h index 7dc15dc0d..ec15cabbd 100644 --- a/src/VecSim/algorithms/svs/svs.h +++ b/src/VecSim/algorithms/svs/svs.h @@ -799,14 +799,37 @@ class SVSIndex : public VecSimIndexAbstract, fl 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. void getDataByLabel( labelType label, std::vector>> &vectors_output) const override { - assert(false && "Not implemented"); + this->log(VecSimCommonStrings::LOG_DEBUG_STRING, + "getDataByLabel: not implemented for SVS, reporting no stored vectors for " + "label %zu", + static_cast(label)); } - - svs::logging::logger_ptr getLogger() const override { return logger_; } -#endif }; #ifdef BUILD_TESTS diff --git a/src/VecSim/vec_sim_index.h b/src/VecSim/vec_sim_index.h index 99fc002a1..5633628e8 100644 --- a/src/VecSim/vec_sim_index.h +++ b/src/VecSim/vec_sim_index.h @@ -327,6 +327,35 @@ struct VecSimIndexAbstract : public VecSimIndexInterface { }; return info; } + /** + * @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 -- 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, for instance — and not the blob originally handed to + * addVector. + * + * 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) + */ + virtual void getDataByLabel(labelType label, + std::vector> &vectors_output) const = 0; + #ifdef BUILD_TESTS void replacePPContainer(PreprocessorsContainerAbstract *newPPContainer) { delete this->preprocessors; @@ -341,24 +370,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 diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index d097b1ae4..5b33f9bec 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 (MOD-17706). +#include "VecSim/algorithms/svs/svs.h" +#endif + #define TIERED_LOG this->backendIndex->log /** @@ -115,6 +120,73 @@ 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`, 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 + * 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 { +#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 + + // 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; +#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); + } + } + } + VecSimTieredIndex(VecSimIndexAbstract *backendIndex_, BruteForceIndex *frontendIndex_, TieredIndexParams tieredParams, std::shared_ptr allocator) diff --git a/tests/unit/test_bruteforce.cpp b/tests/unit/test_bruteforce.cpp index 367326be0..f00f4d8c0 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 4baf526fa..107d8ef06 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..6f4c9b451 100644 --- a/tests/unit/test_hnsw_sq8.cpp +++ b/tests/unit/test_hnsw_sq8.cpp @@ -135,6 +135,46 @@ void HNSWSQ8Test::create_index_test() { EXPECT_EQ(index->basicInfo().algo, VecSimAlgo_HNSWLIB); } +// 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) { + // 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)); + + std::vector> stored; + hnsw_index->getDataByLabel(0, stored); + EXPECT_TRUE(stored.empty()); +} + TYPED_TEST(HNSWSQ8Test, CreateIndex) { this->create_index_test(); } TYPED_TEST(HNSWSQ8Test, RejectStandaloneCosine) { diff --git a/tests/unit/test_hnsw_tiered.cpp b/tests/unit/test_hnsw_tiered.cpp index fc049b34c..aca94224e 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; diff --git a/tests/unit/test_svs.cpp b/tests/unit/test_svs.cpp index 244e0c207..27b381327 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); 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.