From bcedcf93e683b57e7afbe98202b5c549cd9ff07a Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Tue, 18 Aug 2026 14:45:20 +0300 Subject: [PATCH 1/5] [MOD-17688] Add relabelVector API Move the vector(s) stored under a label to a new label, without touching the stored vector data or the index structure. HNSW keys its graph purely on internal ids - links, entry point and levels never mention a label - so a relabel only has to rewrite the two places a label is kept. That makes it O(1) per stored vector, with no distance computations and no graph mutation: a cheap alternative to delete-then-add for callers whose external id changed while the vector itself did not. Implemented for brute force (single/multi), HNSW (single/multi) and tiered HNSW. SVS delegates label management to the external library, so it inherits the interface default that reports "unsupported". The move is rejected, leaving the index untouched, when the source label is absent, the target label is already taken, or the two labels are equal. Rejecting rather than overwriting keeps the caller in control - overwriting the target would silently drop a vector, and unlike addVector there is no replacement data to justify it. Two details worth noting for review: - HNSW assigns idToMetaData[id].label as a field rather than replacing the ElementMetaData struct, whose constructor resets flags to IN_PROCESS. Relabeling an element that is mid-insertion is a reachable state, and struct assignment would reset that flag on an element the ingesting worker is about to unmark, leaving it permanently invisible to queries. - The tiered index moves all four homes of a label (flat lookups, the labelToInsertJobs key, each pending job's own label copy, and HNSW) under one flatIndexGuard-exclusive then mainIndexGuard-exclusive section. The main guard must be exclusive because query threads read getExternalLabel holding only a shared main guard, and ElementMetaData is byte-packed so the label store is not atomic. Re-keying the job map without rewriting job->label would throw std::out_of_range out of a worker thread; a test covers that path. Co-Authored-By: Claude Opus 5 (1M context) --- .../brute_force/brute_force_multi.h | 24 +++ .../brute_force/brute_force_single.h | 21 +++ src/VecSim/algorithms/hnsw/hnsw.h | 44 +++++ .../algorithms/hnsw/hnsw_base_tests_friends.h | 8 + src/VecSim/algorithms/hnsw/hnsw_multi.h | 3 + src/VecSim/algorithms/hnsw/hnsw_single.h | 3 + src/VecSim/algorithms/hnsw/hnsw_tiered.h | 64 +++++++ .../hnsw/hnsw_tiered_tests_friends.h | 4 + src/VecSim/vec_sim.cpp | 4 + src/VecSim/vec_sim.h | 19 ++ src/VecSim/vec_sim_interface.h | 23 +++ tests/unit/test_hnsw.cpp | 102 ++++++++++ tests/unit/test_hnsw_multi.cpp | 46 +++++ tests/unit/test_hnsw_tiered.cpp | 178 ++++++++++++++++++ 14 files changed, 543 insertions(+) diff --git a/src/VecSim/algorithms/brute_force/brute_force_multi.h b/src/VecSim/algorithms/brute_force/brute_force_multi.h index 343faea6b..ff1b39cdb 100644 --- a/src/VecSim/algorithms/brute_force/brute_force_multi.h +++ b/src/VecSim/algorithms/brute_force/brute_force_multi.h @@ -29,6 +29,7 @@ class BruteForceIndex_Multi : public BruteForceIndex { int addVector(const void *vector_data, labelType label) override; int deleteVector(labelType labelType) override; int deleteVectorById(labelType label, idType id) override; + int relabelVector(labelType old_label, labelType new_label) override; double getDistanceFrom_Unsafe(labelType label, const void *vector_data) const override; inline size_t indexLabelCount() const override { return this->labelToIdsLookup.size(); } @@ -221,6 +222,29 @@ int BruteForceIndex_Multi::deleteVectorById(labelType label, return 0; } +template +int BruteForceIndex_Multi::relabelVector(labelType old_label, + labelType new_label) { + if (old_label == new_label) { + return 0; + } + auto old_it = labelToIdsLookup.find(old_label); + if (old_it == labelToIdsLookup.end() || + labelToIdsLookup.find(new_label) != labelToIdsLookup.end()) { + return 0; + } + + // Every id under the label moves, so the whole id vector is re-keyed as-is. + auto ids = std::move(old_it->second); + labelToIdsLookup.erase(old_it); + for (idType id : ids) { + // Keep the id->label direction in sync; `topKQuery` reports results through it. + this->setVectorLabel(id, new_label); + } + labelToIdsLookup.emplace(new_label, std::move(ids)); + return 1; +} + template double BruteForceIndex_Multi::getDistanceFrom_Unsafe(labelType 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..bae14229c 100644 --- a/src/VecSim/algorithms/brute_force/brute_force_single.h +++ b/src/VecSim/algorithms/brute_force/brute_force_single.h @@ -27,6 +27,7 @@ class BruteForceIndex_Single : public BruteForceIndex { int addVector(const void *vector_data, labelType label) override; int deleteVector(labelType label) override; int deleteVectorById(labelType label, idType id) override; + int relabelVector(labelType old_label, labelType new_label) override; double getDistanceFrom_Unsafe(labelType label, const void *vector_data) const override; std::unique_ptr @@ -197,6 +198,26 @@ int BruteForceIndex_Single::deleteVectorById(labelType label return deleteVector(label); } +template +int BruteForceIndex_Single::relabelVector(labelType old_label, + labelType new_label) { + if (old_label == new_label) { + return 0; + } + auto old_it = labelToIdLookup.find(old_label); + if (old_it == labelToIdLookup.end() || + labelToIdLookup.find(new_label) != labelToIdLookup.end()) { + return 0; + } + + const idType id = old_it->second; + labelToIdLookup.erase(old_it); + labelToIdLookup.emplace(new_label, id); + // Keep the id->label direction in sync; `topKQuery` reports results through it. + this->setVectorLabel(id, new_label); + return 1; +} + template double BruteForceIndex_Single::getDistanceFrom_Unsafe(labelType label, diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 90d8ad490..44801c20a 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -319,6 +319,12 @@ class HNSWIndex : public VecSimIndexAbstract, // Remove label from the index. virtual int removeLabel(labelType label) = 0; + // Check whether a label currently maps to at least one element. Note that a label whose + // element was marked deleted is *not* considered to exist, matching `getElementIds`. + virtual bool isLabelExists(labelType label) = 0; + + int relabelVector(labelType old_label, labelType new_label) override; + #ifdef BUILD_TESTS void fitMemory() override { if (maxElements > 0) { @@ -478,6 +484,44 @@ void HNSWIndex::unmarkInProcess(idType internalId) { unmarkAs(internalId); } +/** + * Move every element stored under `old_label` to `new_label`. The graph is keyed purely on internal + * ids - links, entry point and levels never mention a label - so this only has to fix the two + * places a label is kept: `idToMetaData[id].label` (id -> label, read by `getExternalLabel` on + * every query result) and the derived class's label -> id lookup. + * + * Takes `indexDataGuard` exclusively, like `markDelete`, since both containers it mutates are + * guarded by it. A tiered index calling this while holding its main index guard exclusively is + * consistent with the main-guard-then-data-guard order used by `insertVectorToHNSW`. + */ +template +int HNSWIndex::relabelVector(labelType old_label, labelType new_label) { + if (old_label == new_label) { + return 0; + } + std::unique_lock index_data_lock(indexDataGuard); + + if (isLabelExists(new_label)) { + return 0; + } + // Elements that were marked deleted are already out of the label lookup, so they are reported + // as absent here and are left alone - their `idToMetaData` label is still needed by the + // pending swap/repair jobs that reference their id. + auto ids = getElementIds(old_label); + if (ids.empty()) { + return 0; + } + + removeLabel(old_label); + for (idType id : ids) { + // Assign the label field rather than the whole struct: `ElementMetaData`'s constructor + // resets `flags` to IN_PROCESS, which would hide a live element from queries. + idToMetaData[id].label = new_label; + setVectorId(new_label, id); + } + return 1; +} + template void HNSWIndex::lockIndexDataGuard() const { indexDataGuard.lock(); diff --git a/src/VecSim/algorithms/hnsw/hnsw_base_tests_friends.h b/src/VecSim/algorithms/hnsw/hnsw_base_tests_friends.h index cf289dffa..25c33845a 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_base_tests_friends.h +++ b/src/VecSim/algorithms/hnsw/hnsw_base_tests_friends.h @@ -24,3 +24,11 @@ INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_deleteFromHNSWMultiLevels_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTest_deleteFromHNSWWithRepairJobExec_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTest_swapJobBasic_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_deleteInplaceAvoidUpdatedMarkedDeleted_Test) +INDEX_TEST_FRIEND_CLASS(HNSWTest_relabelVector_Test) +INDEX_TEST_FRIEND_CLASS(HNSWTest_relabelVectorRejects_Test) +INDEX_TEST_FRIEND_CLASS(HNSWTest_relabelVectorMarkedDeleted_Test) +INDEX_TEST_FRIEND_CLASS(HNSWMultiTest_relabelVectorMulti_Test) +INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorFlatOnly_Test) +INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorBothTiers_Test) +INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorRejects_Test) +INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorDuringIngestion_Test) diff --git a/src/VecSim/algorithms/hnsw/hnsw_multi.h b/src/VecSim/algorithms/hnsw/hnsw_multi.h index 736045ccd..838556b0d 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_multi.h +++ b/src/VecSim/algorithms/hnsw/hnsw_multi.h @@ -125,6 +125,9 @@ class HNSWIndex_Multi : public HNSWIndex { return getDistanceFromInternal(label, vector_data); } int removeLabel(labelType label) override { return labelLookup.erase(label); } + bool isLabelExists(labelType label) override { + return labelLookup.find(label) != labelLookup.end(); + } }; /** diff --git a/src/VecSim/algorithms/hnsw/hnsw_single.h b/src/VecSim/algorithms/hnsw/hnsw_single.h index 99fcf7652..32e12c24a 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_single.h +++ b/src/VecSim/algorithms/hnsw/hnsw_single.h @@ -95,6 +95,9 @@ class HNSWIndex_Single : public HNSWIndex { return getDistanceFromInternal(label, vector_data); } int removeLabel(labelType label) override { return labelLookup.erase(label); } + bool isLabelExists(labelType label) override { + return labelLookup.find(label) != labelLookup.end(); + } }; /** diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index b90ba8e69..9574cbd03 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -213,6 +213,7 @@ class TieredHNSWIndex : public VecSimTieredIndex { int addVector(const void *blob, labelType label) override; int deleteVector(labelType label) override; + int relabelVector(labelType old_label, labelType new_label) override; size_t getNumMarkedDeleted() const override { return this->getHNSWIndex()->getNumMarkedDeleted(); } @@ -915,6 +916,69 @@ int TieredHNSWIndex::deleteVector(labelType label) { return num_deleted_vectors; } +/** + * Move a label across every place the tiered index records one. Unlike add/delete this needs no + * jobs and no vector data movement - the swap/repair machinery is keyed purely on internal ids, so + * it is unaffected. + * + * A label can legitimately live in *both* tiers at once: `executeInsertJob` inserts into HNSW + * before removing the vector from the flat buffer, so all four updates below are independently + * guarded by their own existence check rather than treated as mutually exclusive. + * + * Locking: `flatIndexGuard` exclusively (it guards the flat buffer's lookups, `labelToInsertJobs` + * and - per the note in `executeInsertJob` - the job fields themselves), then the main index guard + * exclusively. That is the flat-then-main order used by `insertVectorToHNSW` and `topKQueryImp`, + * so it cannot deadlock against them; repair and swap jobs only ever take the main guard. + * The main guard must be *exclusive* rather than shared: query threads read + * `getExternalLabel` under a shared main guard, and `ElementMetaData` is byte-packed, so its + * `label` field can be unaligned and its store is not atomic. + */ +template +int TieredHNSWIndex::relabelVector(labelType old_label, labelType new_label) { + if (old_label == new_label) { + return 0; + } + + this->flatIndexGuard.lock(); + this->lockMainIndexGuard(); + + auto *hnsw_index = this->getHNSWIndex(); + int ret = 0; + // Reject if the target is taken anywhere, so that a partially applied move is impossible: a + // free target in all three homes means none of the updates below can collide. + const bool target_taken = + this->frontendIndex->isLabelExists(new_label) || + this->labelToInsertJobs.find(new_label) != this->labelToInsertJobs.end() || + hnsw_index->isLabelExists(new_label); + if (!target_taken) { + if (this->frontendIndex->isLabelExists(old_label)) { + ret |= this->frontendIndex->relabelVector(old_label, new_label); + } + + // Re-key the pending insert jobs *and* rewrite each job's own copy of the label. Both must + // move together: `executeInsertJob` indexes into HNSW under `job->label` and then looks the + // job up with `labelToInsertJobs.at(job->label)`, so a half-applied move either indexes the + // vector under the stale label or throws out of the worker thread. + auto jobs_it = this->labelToInsertJobs.find(old_label); + if (jobs_it != this->labelToInsertJobs.end()) { + auto jobs = std::move(jobs_it->second); + this->labelToInsertJobs.erase(jobs_it); + for (HNSWInsertJob *job : jobs) { + job->label = new_label; + } + this->labelToInsertJobs.emplace(new_label, std::move(jobs)); + } + + // `relabelVector` takes the HNSW index data guard internally, which is the same + // main-guard-then-data-guard order that `insertVectorToHNSW` uses. + ret |= hnsw_index->relabelVector(old_label, new_label); + } + + this->unlockMainIndexGuard(); + this->flatIndexGuard.unlock(); + return ret; +} + // `getDistanceFrom` returns the minimum distance between the given blob and the vector with the // given label. If the label doesn't exist, the distance will be NaN. // Therefore, it's better to just call `getDistanceFrom` on both indexes and return the minimum diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h b/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h index d4d5cd999..1b8dcebb0 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h @@ -77,3 +77,7 @@ INDEX_TEST_FRIEND_CLASS(BM_VecSimBasics) INDEX_TEST_FRIEND_CLASS(BM_VecSimCommon) friend class BM_IncomingEdgesBase; +INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorFlatOnly_Test) +INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorBothTiers_Test) +INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorRejects_Test) +INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorDuringIngestion_Test) diff --git a/src/VecSim/vec_sim.cpp b/src/VecSim/vec_sim.cpp index e407228f9..27794c1de 100644 --- a/src/VecSim/vec_sim.cpp +++ b/src/VecSim/vec_sim.cpp @@ -230,6 +230,10 @@ extern "C" int VecSimIndex_DeleteVector(VecSimIndex *index, size_t label) { return index->deleteVector(label); } +extern "C" int VecSimIndex_RelabelVector(VecSimIndex *index, size_t old_label, size_t new_label) { + return index->relabelVector(old_label, new_label); +} + extern "C" double VecSimIndex_GetDistanceFrom_Unsafe(VecSimIndex *index, size_t label, const void *blob) { return index->getDistanceFrom_Unsafe(label, blob); diff --git a/src/VecSim/vec_sim.h b/src/VecSim/vec_sim.h index b6b0110da..72157039a 100644 --- a/src/VecSim/vec_sim.h +++ b/src/VecSim/vec_sim.h @@ -72,6 +72,25 @@ int VecSimIndex_AddVector(VecSimIndex *index, const void *blob, size_t label); */ int VecSimIndex_DeleteVector(VecSimIndex *index, size_t label); +/** + * @brief Move the vector(s) stored under `old_label` to `new_label`, leaving the stored vector data + * and the index structure untouched. + * + * Only label bookkeeping is rewritten, so this costs O(1) per stored vector - no distance + * computations and, for HNSW, no graph mutation. Intended for callers whose external id changed + * while the vector did not, as a cheap replacement for delete-then-add. + * + * The move is rejected, leaving the index untouched, if `old_label` does not exist, if `new_label` + * is already in use, or if the labels are equal. Not all index types support this; unsupported + * types report 0. + * + * @param index the index holding the vector(s). + * @param old_label the label currently holding the vector(s). + * @param new_label the label to move them to. + * @return 1 if the label was moved, 0 otherwise. + */ +int VecSimIndex_RelabelVector(VecSimIndex *index, size_t old_label, size_t new_label); + /** * @brief Calculate the distance of a vector from an index to a vector. This function assumes that * the vector fits the index - its type and dimension are the same as the index's, and if the diff --git a/src/VecSim/vec_sim_interface.h b/src/VecSim/vec_sim_interface.h index 85f40c6df..49a637920 100644 --- a/src/VecSim/vec_sim_interface.h +++ b/src/VecSim/vec_sim_interface.h @@ -54,6 +54,29 @@ struct VecSimIndexInterface : public VecsimBaseObject { */ virtual int deleteVector(labelType label) = 0; + /** + * @brief Move the vector(s) stored under `old_label` to `new_label`, without touching the + * stored vector data or the index structure. + * + * Only the label bookkeeping changes, so this is O(1) per stored vector: no distance + * computations, no graph mutation, and no invalidation of internal ids. It is the cheap + * alternative to delete-then-add for callers whose external id changed while the vector + * itself did not. + * + * The operation is rejected (returns 0, index left untouched) if `old_label` is not present, + * if `new_label` is already taken, or if the two labels are equal. Rejecting rather than + * overwriting keeps the caller in control: overwriting `new_label` would silently drop a + * vector, and unlike `addVector` there is no replacement data to justify it. + * + * The default implementation reports "unsupported" so that index types which delegate label + * management to an external library are not forced to implement it. + * + * @param old_label the label currently holding the vector(s). + * @param new_label the label to move them to. + * @return 1 if the label was moved, 0 otherwise. + */ + virtual int relabelVector(labelType old_label, labelType new_label) { return 0; } + /** * @brief Calculate the distance of a vector from an index to a vector. * @param index the index from which the first vector is located, and that defines the distance diff --git a/tests/unit/test_hnsw.cpp b/tests/unit/test_hnsw.cpp index dfd0e1083..299e376de 100644 --- a/tests/unit/test_hnsw.cpp +++ b/tests/unit/test_hnsw.cpp @@ -2368,3 +2368,105 @@ TYPED_TEST(HNSWTest, FitMemoryTest) { VecSimIndex_Free(index); } + +TYPED_TEST(HNSWTest, relabelVector) { + size_t dim = 4; + size_t n = 10; + HNSWParams params = {.dim = dim, .metric = VecSimMetric_L2, .M = 16, .efConstruction = 200}; + VecSimIndex *index = this->CreateNewIndex(params); + auto *hnsw_index = this->CastToHNSW(index); + + for (size_t i = 0; i < n; i++) { + GenerateAndAddVector(index, dim, i, i); + } + + // Capture the state that a relabel must preserve: the internal id, and the distance from a + // query, which together prove that neither the stored data nor the graph position moved. + const labelType old_label = 3; + const labelType new_label = 100; + auto ids_before = hnsw_index->getElementIds(old_label); + ASSERT_EQ(ids_before.size(), 1); + TEST_DATA_T query[dim]; + GenerateVector(query, dim, old_label); + const double dist_before = hnsw_index->getDistanceFrom_Unsafe(old_label, query); + + ASSERT_EQ(VecSimIndex_RelabelVector(index, old_label, new_label), 1); + + // Nothing was added or removed. + ASSERT_EQ(VecSimIndex_IndexSize(index), n); + ASSERT_EQ(index->indexLabelCount(), n); + ASSERT_TRUE(hnsw_index->checkIntegrity().valid_state); + + // The old label is gone, the new one holds the very same element. + ASSERT_FALSE(hnsw_index->isLabelExists(old_label)); + ASSERT_TRUE(hnsw_index->isLabelExists(new_label)); + ASSERT_EQ(hnsw_index->getElementIds(new_label), ids_before); + ASSERT_EQ(hnsw_index->getExternalLabel(ids_before[0]), new_label); + ASSERT_EQ(hnsw_index->getDistanceFrom_Unsafe(new_label, query), dist_before); + ASSERT_TRUE(std::isnan(hnsw_index->getDistanceFrom_Unsafe(old_label, query))); + + // The element is still reachable by search, under the new label and with an unchanged score. + // This also guards against clobbering the element's flags with IN_PROCESS, which would make a + // live element invisible to queries. + auto verify_res = [&](size_t id, double score, size_t rank) { + ASSERT_EQ(id, new_label); + ASSERT_EQ(score, dist_before); + }; + runTopKSearchTest(index, query, 1, verify_res); + + VecSimIndex_Free(index); +} + +TYPED_TEST(HNSWTest, relabelVectorRejects) { + size_t dim = 4; + size_t n = 5; + HNSWParams params = {.dim = dim, .metric = VecSimMetric_L2}; + VecSimIndex *index = this->CreateNewIndex(params); + auto *hnsw_index = this->CastToHNSW(index); + + for (size_t i = 0; i < n; i++) { + GenerateAndAddVector(index, dim, i, i); + } + + // A missing source, an occupied target and a no-op move are all rejected, and none of them may + // leave the index in a modified state. + ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 100), 0); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 2), 0); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 1), 0); + + ASSERT_EQ(VecSimIndex_IndexSize(index), n); + ASSERT_EQ(index->indexLabelCount(), n); + ASSERT_TRUE(hnsw_index->checkIntegrity().valid_state); + for (size_t i = 0; i < n; i++) { + ASSERT_TRUE(hnsw_index->isLabelExists(i)); + } + ASSERT_FALSE(hnsw_index->isLabelExists(100)); + + VecSimIndex_Free(index); +} + +TYPED_TEST(HNSWTest, relabelVectorMarkedDeleted) { + size_t dim = 4; + HNSWParams params = {.dim = dim, .metric = VecSimMetric_L2}; + VecSimIndex *index = this->CreateNewIndex(params); + auto *hnsw_index = this->CastToHNSW(index); + + GenerateAndAddVector(index, dim, 0, 0); + GenerateAndAddVector(index, dim, 1, 1); + + auto deleted_ids = hnsw_index->markDelete(0); + ASSERT_EQ(deleted_ids.size(), 1); + + // A marked-deleted element is out of the label lookup, so it is reported as absent and left + // alone - its `idToMetaData` label is still needed by the swap/repair jobs holding its id. + ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 100), 0); + ASSERT_EQ(hnsw_index->getExternalLabel(deleted_ids[0]), 0); + ASSERT_FALSE(hnsw_index->isLabelExists(100)); + + // A live label in the same index still relabels fine. + ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 101), 1); + ASSERT_TRUE(hnsw_index->isLabelExists(101)); + ASSERT_TRUE(hnsw_index->checkIntegrity().valid_state); + + VecSimIndex_Free(index); +} diff --git a/tests/unit/test_hnsw_multi.cpp b/tests/unit/test_hnsw_multi.cpp index 9b6b2b974..963a895dd 100644 --- a/tests/unit/test_hnsw_multi.cpp +++ b/tests/unit/test_hnsw_multi.cpp @@ -1626,3 +1626,49 @@ TYPED_TEST(HNSWMultiTest, markDelete) { VecSimBatchIterator_Free(batchIterator); VecSimIndex_Free(index); } + +TYPED_TEST(HNSWMultiTest, relabelVectorMulti) { + size_t dim = 4; + size_t per_label = 3; + HNSWParams params = {.dim = dim, .metric = VecSimMetric_L2}; + VecSimIndex *index = this->CreateNewIndex(params); + auto *hnsw_index = this->CastToHNSW(index); + + // Two labels, each holding several vectors. + for (size_t i = 0; i < per_label; i++) { + GenerateAndAddVector(index, dim, 0, i); + GenerateAndAddVector(index, dim, 1, i + 10); + } + ASSERT_EQ(VecSimIndex_IndexSize(index), per_label * 2); + ASSERT_EQ(index->indexLabelCount(), 2); + + auto ids_before = hnsw_index->getElementIds(0); + ASSERT_EQ(ids_before.size(), per_label); + + ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 100), 1); + + // Every id under the label moves together, keeping its order, and the untouched label is + // unaffected. + ASSERT_EQ(VecSimIndex_IndexSize(index), per_label * 2); + ASSERT_EQ(index->indexLabelCount(), 2); + ASSERT_FALSE(hnsw_index->isLabelExists(0)); + ASSERT_EQ(hnsw_index->getElementIds(100), ids_before); + for (idType id : ids_before) { + ASSERT_EQ(hnsw_index->getExternalLabel(id), 100); + } + ASSERT_EQ(hnsw_index->getElementIds(1).size(), per_label); + ASSERT_TRUE(hnsw_index->checkIntegrity().valid_state); + + // All `per_label` vectors are still searchable, now reported under the new label. + TEST_DATA_T query[dim]; + GenerateVector(query, dim, 0); + auto verify_res = [&](size_t id, double score, size_t rank) { ASSERT_EQ(id, 100); }; + runTopKSearchTest(index, query, 1, verify_res); + + // Moving onto an occupied label is rejected without disturbing either label. + ASSERT_EQ(VecSimIndex_RelabelVector(index, 100, 1), 0); + ASSERT_EQ(hnsw_index->getElementIds(100).size(), per_label); + ASSERT_EQ(hnsw_index->getElementIds(1).size(), per_label); + + VecSimIndex_Free(index); +} diff --git a/tests/unit/test_hnsw_tiered.cpp b/tests/unit/test_hnsw_tiered.cpp index a0927790b..ae75a659c 100644 --- a/tests/unit/test_hnsw_tiered.cpp +++ b/tests/unit/test_hnsw_tiered.cpp @@ -4587,3 +4587,181 @@ TYPED_TEST(HNSWTieredIndexTestBasic, HNSWResize) { hnsw_index->indexMetaDataCapacity() + tiered_index->frontendIndex->indexMetaDataCapacity()); } + +TYPED_TEST(HNSWTieredIndexTestBasic, relabelVectorFlatOnly) { + size_t dim = 4; + HNSWParams params = { + .type = TypeParam::get_index_type(), .dim = dim, .metric = VecSimMetric_L2, .multi = false}; + VecSimParams hnsw_params = CreateParams(params); + auto mock_thread_pool = tieredIndexMock(); + auto *tiered_index = this->CreateTieredHNSWIndex(hnsw_params, mock_thread_pool); + auto *frontend_index = this->GetFlatIndex(tiered_index); + auto *hnsw_index = this->CastToHNSW(tiered_index); + + // The vector lands in the flat buffer with a pending insert job; nothing is in HNSW yet. + GenerateAndAddVector(tiered_index, dim, 7, 7); + ASSERT_EQ(frontend_index->indexSize(), 1); + ASSERT_EQ(hnsw_index->indexSize(), 0); + ASSERT_EQ(tiered_index->labelToInsertJobs.at(7).size(), 1); + const idType flat_id = tiered_index->labelToInsertJobs.at(7)[0]->id; + + ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 7, 70), 1); + + // The flat tier moved, and so did both halves of the job bookkeeping: the map key and the + // job's own copy of the label. A half-applied move would make the worker below either index + // the vector under the stale label or throw out of `labelToInsertJobs.at`. + ASSERT_TRUE(frontend_index->isLabelExists(70)); + ASSERT_FALSE(frontend_index->isLabelExists(7)); + ASSERT_EQ(tiered_index->labelToInsertJobs.count(7), 0); + ASSERT_EQ(tiered_index->labelToInsertJobs.at(70).size(), 1); + ASSERT_EQ(tiered_index->labelToInsertJobs.at(70)[0]->label, 70); + ASSERT_EQ(tiered_index->labelToInsertJobs.at(70)[0]->id, flat_id); + ASSERT_EQ(VecSimIndex_IndexSize(tiered_index), 1); + + // Draining the job must ingest the vector into HNSW under the *new* label and clear the flat + // tier, exactly as it would have for a label that was never moved. + ASSERT_EQ(mock_thread_pool.jobQ.size(), 1); + mock_thread_pool.thread_iteration(); + ASSERT_EQ(frontend_index->indexSize(), 0); + ASSERT_EQ(hnsw_index->indexSize(), 1); + ASSERT_TRUE(hnsw_index->isLabelExists(70)); + ASSERT_FALSE(hnsw_index->isLabelExists(7)); + ASSERT_EQ(tiered_index->labelToInsertJobs.count(70), 0); + ASSERT_TRUE(hnsw_index->checkIntegrity().valid_state); + + TEST_DATA_T query[dim]; + GenerateVector(query, dim, 7); + auto verify_res = [&](size_t id, double score, size_t rank) { + ASSERT_EQ(id, 70); + ASSERT_EQ(score, 0); + }; + runTopKSearchTest(tiered_index, query, 1, verify_res); +} + +TYPED_TEST(HNSWTieredIndexTestBasic, relabelVectorBothTiers) { + size_t dim = 4; + HNSWParams params = { + .type = TypeParam::get_index_type(), .dim = dim, .metric = VecSimMetric_L2, .multi = false}; + VecSimParams hnsw_params = CreateParams(params); + auto mock_thread_pool = tieredIndexMock(); + auto *tiered_index = this->CreateTieredHNSWIndex(hnsw_params, mock_thread_pool); + auto *frontend_index = this->GetFlatIndex(tiered_index); + auto *hnsw_index = this->CastToHNSW(tiered_index); + + // Reproduce the ingestion window: `executeInsertJob` inserts into HNSW *before* removing the + // vector from the flat buffer, so a label is legitimately live in both tiers at once. Build + // that state directly so the test is deterministic. + GenerateAndAddVector(tiered_index, dim, 7, 7); + TEST_DATA_T vector[dim]; + GenerateVector(vector, dim, 7); + hnsw_index->addVector(vector, 7); + ASSERT_EQ(frontend_index->indexSize(), 1); + ASSERT_EQ(hnsw_index->indexSize(), 1); + + ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 7, 70), 1); + + // Both tiers must move - an implementation treating them as mutually exclusive would leave one + // stale copy behind under the old label. + ASSERT_TRUE(frontend_index->isLabelExists(70)); + ASSERT_FALSE(frontend_index->isLabelExists(7)); + ASSERT_TRUE(hnsw_index->isLabelExists(70)); + ASSERT_FALSE(hnsw_index->isLabelExists(7)); + ASSERT_EQ(tiered_index->labelToInsertJobs.at(70)[0]->label, 70); + ASSERT_TRUE(hnsw_index->checkIntegrity().valid_state); +} + +TYPED_TEST(HNSWTieredIndexTestBasic, relabelVectorRejects) { + size_t dim = 4; + HNSWParams params = { + .type = TypeParam::get_index_type(), .dim = dim, .metric = VecSimMetric_L2, .multi = false}; + VecSimParams hnsw_params = CreateParams(params); + auto mock_thread_pool = tieredIndexMock(); + auto *tiered_index = this->CreateTieredHNSWIndex(hnsw_params, mock_thread_pool); + auto *frontend_index = this->GetFlatIndex(tiered_index); + auto *hnsw_index = this->CastToHNSW(tiered_index); + + // Label 1 is ingested into HNSW, label 2 stays pending in the flat buffer. The target of a + // relabel must be free in *both* tiers and in the pending-job map. + GenerateAndAddVector(tiered_index, dim, 1, 1); + mock_thread_pool.thread_iteration(); + GenerateAndAddVector(tiered_index, dim, 2, 2); + ASSERT_EQ(hnsw_index->indexSize(), 1); + ASSERT_EQ(frontend_index->indexSize(), 1); + + ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 2, 1), 0); // target lives in HNSW + ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 1, 2), 0); // target lives in flat + jobs + ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 1, 1), 0); // no-op + ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 42, 43), 0); // source absent + + // Every rejection left both tiers and the job map untouched. + ASSERT_TRUE(hnsw_index->isLabelExists(1)); + ASSERT_TRUE(frontend_index->isLabelExists(2)); + ASSERT_EQ(tiered_index->labelToInsertJobs.at(2).size(), 1); + ASSERT_EQ(tiered_index->labelToInsertJobs.at(2)[0]->label, 2); + ASSERT_EQ(VecSimIndex_IndexSize(tiered_index), 2); + ASSERT_TRUE(hnsw_index->checkIntegrity().valid_state); +} + +// Relabel racing against live ingestion by the worker threads. The point of interest is the +// invariant that `addVector` and `deleteVector` rely on when they reach for +// `labelToInsertJobs.at(label)` behind an `isLabelExists(label)` guard, and that +// `executeInsertJob` relies on for `labelToInsertJobs.at(job->label)`: a label present in the flat +// buffer must be a key in the job map. A relabel that moved the flat entry without re-keying the +// job map (or without rewriting `job->label`) breaks it, and the resulting `std::out_of_range` +// would be thrown inside a worker thread - i.e. it would terminate the process, not fail an +// assertion. So reaching the end of this test at all is the real assertion. +TYPED_TEST(HNSWTieredIndexTestBasic, relabelVectorDuringIngestion) { + size_t dim = 4; + size_t n = 500; + // The offset that maps an original label to its relabeled value; kept larger than `n` so the + // two ranges cannot overlap and make a relabel fail on an occupied target. + const labelType relabel_offset = 10000; + + HNSWParams params = { + .type = TypeParam::get_index_type(), .dim = dim, .metric = VecSimMetric_L2, .multi = false}; + VecSimParams hnsw_params = CreateParams(params); + auto mock_thread_pool = tieredIndexMock(); + auto *tiered_index = this->CreateTieredHNSWIndex(hnsw_params, mock_thread_pool); + + mock_thread_pool.init_threads(); + + // Interleave adds with relabels of already-added labels, so that a relabel can land while a + // worker is anywhere in `executeInsertJob` for that same label - including the window where the + // vector is live in both tiers at once. + size_t relabeled = 0; + for (size_t i = 0; i < n; i++) { + GenerateAndAddVector(tiered_index, dim, i, i); + if (i > 0) { + relabeled += tiered_index->relabelVector(i - 1, i - 1 + relabel_offset); + } + } + relabeled += tiered_index->relabelVector(n - 1, n - 1 + relabel_offset); + + mock_thread_pool.thread_pool_join(); + + // Every relabel targeted a distinct, unoccupied label of a vector known to exist, so all of + // them must have been applied - otherwise the label accounting below would pass trivially. + ASSERT_EQ(relabeled, n); + + // The index is fully ingested, holds exactly the relabeled range, and none of the original + // labels survived anywhere. + auto *hnsw_index = this->CastToHNSW(tiered_index); + ASSERT_EQ(tiered_index->indexSize(), n); + ASSERT_EQ(tiered_index->backendIndex->indexSize(), n); + ASSERT_EQ(tiered_index->frontendIndex->indexSize(), 0); + ASSERT_EQ(tiered_index->labelToInsertJobs.size(), 0); + ASSERT_EQ(tiered_index->indexLabelCount(), n); + for (size_t i = 0; i < n; i++) { + ASSERT_TRUE(hnsw_index->isLabelExists(i + relabel_offset)) << "missing label " << i; + ASSERT_FALSE(hnsw_index->isLabelExists(i)) << "stale label " << i; + } + ASSERT_TRUE(hnsw_index->checkIntegrity().valid_state); + + // Each moved label still resolves to its own vector, so no relabel crossed wires. + for (size_t i = 0; i < n; i++) { + TEST_DATA_T expected[dim]; + GenerateVector(expected, dim, i); + ASSERT_EQ(tiered_index->getDistanceFrom_Unsafe(i + relabel_offset, expected), 0) + << "label " << i + relabel_offset << " does not hold its original vector"; + } +} From d45a26be35aeada4baa85a8b8b829086ea15755b Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Tue, 18 Aug 2026 14:58:26 +0300 Subject: [PATCH 2/5] Add a RelabelLabel benchmark Measures relabelVector on the fp32 single and multi basics suites, registered for BF, HNSW and tiered HNSW. Reviewers can compare it against the AddLabel and DeleteLabel rows in the same run - same dataset, same harness - which is the pair relabelVector replaces for callers whose external id changed while the vector did not. Reported in microseconds rather than the milliseconds the neighbouring add/delete benchmarks use, since a relabel only rewrites label bookkeeping. The iteration count is fixed rather than left to the harness because each iteration consumes one source label: overrunning the label range would make relabelVector reject the call and silently time a no-op. The relabeled_ratio counter surfaces that - anything below 1 means the reported time is not measuring the relabel work. Each run restores the original labels afterwards, since benchmark order affects results in this suite and the tiered index shares its HNSW with INDEX_HNSW. Co-Authored-By: Claude Opus 5 (1M context) --- .../bm_basics_initialize_fp32.h | 7 +++ tests/benchmark/bm_vecsim_basics.h | 54 +++++++++++++++++++ .../run_files/bm_basics_multi_fp32.cpp | 1 + .../run_files/bm_basics_single_fp32.cpp | 1 + 4 files changed, 63 insertions(+) diff --git a/tests/benchmark/bm_initialization/bm_basics_initialize_fp32.h b/tests/benchmark/bm_initialization/bm_basics_initialize_fp32.h index 408fa1a23..3e994f33d 100644 --- a/tests/benchmark/bm_initialization/bm_basics_initialize_fp32.h +++ b/tests/benchmark/bm_initialization/bm_basics_initialize_fp32.h @@ -81,3 +81,10 @@ BENCHMARK_REGISTER_F(BM_VecSimBasics, BM_DELETE_LABEL_ASYNC) ->Arg(100) ->Arg(BM_VecSimGeneral::block_size) ->ArgName("SwapJobsThreshold"); + +// RelabelLabel +BENCHMARK_TEMPLATE_DEFINE_F(BM_VecSimBasics, BM_RELABEL_LABEL, fp32_index_t) +(benchmark::State &st) { RelabelLabel(st); } +REGISTER_RelabelLabel(BM_RELABEL_LABEL, INDEX_BF); +REGISTER_RelabelLabel(BM_RELABEL_LABEL, INDEX_HNSW); +REGISTER_RelabelLabel(BM_RELABEL_LABEL, INDEX_TIERED_HNSW); diff --git a/tests/benchmark/bm_vecsim_basics.h b/tests/benchmark/bm_vecsim_basics.h index b81a6b0d5..2405e69b8 100644 --- a/tests/benchmark/bm_vecsim_basics.h +++ b/tests/benchmark/bm_vecsim_basics.h @@ -39,6 +39,13 @@ class BM_VecSimBasics : public BM_VecSimCommon { template static void DeleteLabel(algo_t *index, benchmark::State &st); + // Move one label to a fresh label in each iteration. This only rewrites label bookkeeping - + // no vector data is copied and, for HNSW, the graph is untouched - so it is expected to be + // orders of magnitude cheaper than the AddLabel + DeleteLabel pair it replaces for callers + // whose external id changed while the vector did not. Reported in microseconds for that + // reason, whereas the add/delete benchmarks above are reported in milliseconds. + static void RelabelLabel(benchmark::State &st); + static void Range_BF(benchmark::State &st); static void Range_HNSW(benchmark::State &st); @@ -266,6 +273,41 @@ void BM_VecSimBasics::DeleteLabel_AsyncRepair(benchmark::State &st assert(VecSimIndex_IndexSize(tiered_index) == N_VECTORS); } +template +void BM_VecSimBasics::RelabelLabel(benchmark::State &st) { + auto index = GET_INDEX(st.range(0)); + const size_t initial_label_count = index->indexLabelCount(); + const size_t initial_index_size = VecSimIndex_IndexSize(index); + + // The loaded index labels occupy [0, initial_label_count), so every target taken from + // initial_label_count upwards is free. That matters because relabelVector rejects an occupied + // target, and a rejected call would time a no-op instead of the real work. + const labelType target_base = initial_label_count; + + size_t attempted = 0; + size_t moved = 0; + for (auto _ : st) { + moved += VecSimIndex_RelabelVector(index, attempted, target_base + attempted); + attempted++; + } + + // Restore the original labels. Benchmark order affects results in this suite, so the following + // benchmarks must see the label range they were written against. Note that for the tiered index + // this also restores what INDEX_HNSW sees, since the tiered index wraps that same HNSW index. + // Restoring an attempt that was rejected above is itself a harmless no-op. + for (size_t i = 0; i < attempted; i++) { + VecSimIndex_RelabelVector(index, target_base + i, i); + } + + // A ratio below 1 means some timed calls were rejected and the reported time is not a + // measurement of the relabel work - most likely the iteration count outgrew the label range. + st.counters["relabeled_ratio"] = (double)moved / (double)attempted; + st.counters["vectors_per_label"] = (double)initial_index_size / (double)initial_label_count; + + assert(index->indexLabelCount() == initial_label_count); + assert(VecSimIndex_IndexSize(index) == initial_index_size); +} + template void BM_VecSimBasics::Range_BF(benchmark::State &st) { double radius = (1.0 / 100.0) * (double)st.range(0); @@ -438,3 +480,15 @@ void BM_VecSimBasics::UpdateAtBlockSize(benchmark::State &st) { BENCHMARK_REGISTER_F(BM_VecSimBasics, BM_FUNC) \ ->UNIT_AND_ITERATIONS->Arg(VecSimAlgo) \ ->ArgName(#VecSimAlgo) + +// A relabel is label bookkeeping only, so it lands orders of magnitude below the add/delete +// benchmarks - hence microseconds rather than the shared UNIT_AND_ITERATIONS milliseconds. The +// iteration count stays fixed (rather than letting the harness choose) because each iteration +// consumes one source label, and overrunning the label range would silently turn timed work into +// rejected no-ops. +#define REGISTER_RelabelLabel(BM_FUNC, VecSimAlgo) \ + BENCHMARK_REGISTER_F(BM_VecSimBasics, BM_FUNC) \ + ->Unit(benchmark::kMicrosecond) \ + ->Iterations(BM_VecSimGeneral::block_size) \ + ->Arg(VecSimAlgo) \ + ->ArgName(#VecSimAlgo) diff --git a/tests/benchmark/run_files/bm_basics_multi_fp32.cpp b/tests/benchmark/run_files/bm_basics_multi_fp32.cpp index 28c938f0f..d12aa9d61 100644 --- a/tests/benchmark/run_files/bm_basics_multi_fp32.cpp +++ b/tests/benchmark/run_files/bm_basics_multi_fp32.cpp @@ -26,6 +26,7 @@ const char *BM_VecSimGeneral::test_queries_file = #define BM_ADD_LABEL CONCAT_WITH_UNDERSCORE_ARCH(AddLabel, Multi) #define BM_ADD_LABEL_ASYNC CONCAT_WITH_UNDERSCORE_ARCH(AddLabel_Async, Multi) #define BM_DELETE_LABEL_ASYNC CONCAT_WITH_UNDERSCORE_ARCH(DeleteLabel_Async, Multi) +#define BM_RELABEL_LABEL CONCAT_WITH_UNDERSCORE_ARCH(RelabelLabel, Multi) DEFINE_DELETE_LABEL(BM_FUNC_NAME(DeleteLabel, BF), fp32_index_t, BruteForceIndex_Multi, float, float, INDEX_BF) diff --git a/tests/benchmark/run_files/bm_basics_single_fp32.cpp b/tests/benchmark/run_files/bm_basics_single_fp32.cpp index aa931e901..37915235f 100644 --- a/tests/benchmark/run_files/bm_basics_single_fp32.cpp +++ b/tests/benchmark/run_files/bm_basics_single_fp32.cpp @@ -24,6 +24,7 @@ const char *BM_VecSimGeneral::test_queries_file = #define BM_ADD_LABEL CONCAT_WITH_UNDERSCORE_ARCH(AddLabel, Single) #define BM_ADD_LABEL_ASYNC CONCAT_WITH_UNDERSCORE_ARCH(AddLabel, Async, Single) #define BM_DELETE_LABEL_ASYNC CONCAT_WITH_UNDERSCORE_ARCH(DeleteLabel_Async, Single) +#define BM_RELABEL_LABEL CONCAT_WITH_UNDERSCORE_ARCH(RelabelLabel, Single) DEFINE_DELETE_LABEL(BM_FUNC_NAME(DeleteLabel, BF), fp32_index_t, BruteForceIndex_Single, float, float, INDEX_BF) From 566ca779ec203f8bcd0da771e109e07253dbc529 Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Tue, 18 Aug 2026 15:40:09 +0300 Subject: [PATCH 3/5] Cover the relabelVector paths codecov flagged The patch report on #1017 listed 16 uncovered lines, all in paths the existing tests reach around rather than through: - brute_force_multi.h (13 lines, 0%): BruteForceIndex_Multi::relabelVector was never called. The tiered tests build single-value indexes, so their flat tier is BruteForceIndex_Single. - brute_force_single.h (2 lines): its own same-label and rejection returns. The tiered caller validates the target across all three label homes before delegating, so the flat index's own guards never fire from that path. - vec_sim_interface.h (1 line): the "unsupported" default, which every tested index type overrides. Adds direct brute force tests for both single and multi - happy path plus all three rejection cases - and an SVS test pinning the unsupported default. The multi rejection case is the one that matters most there: an accepted move onto an occupied label would silently merge two labels' vectors. Co-Authored-By: Claude Opus 5 (1M context) --- tests/unit/test_bruteforce.cpp | 64 +++++++++++++++++++++++ tests/unit/test_bruteforce_multi.cpp | 76 ++++++++++++++++++++++++++++ tests/unit/test_svs.cpp | 22 ++++++++ 3 files changed, 162 insertions(+) diff --git a/tests/unit/test_bruteforce.cpp b/tests/unit/test_bruteforce.cpp index abf7e9855..021ec6bb2 100644 --- a/tests/unit/test_bruteforce.cpp +++ b/tests/unit/test_bruteforce.cpp @@ -1674,3 +1674,67 @@ TYPED_TEST(BruteForceTest, FitMemoryTest) { VecSimIndex_Free(index); } + +TYPED_TEST(BruteForceTest, relabelVector) { + size_t dim = 4; + size_t n = 10; + BFParams params = {.dim = dim, .metric = VecSimMetric_L2}; + VecSimIndex *index = this->CreateNewIndex(params); + + for (size_t i = 0; i < n; i++) { + GenerateAndAddVector(index, dim, i, i); + } + + const labelType old_label = 3; + const labelType new_label = 100; + TEST_DATA_T query[dim]; + GenerateVector(query, dim, old_label); + // A relabel must not move the stored data, so the distance from the label's own vector stays 0. + ASSERT_EQ(VecSimIndex_GetDistanceFrom_Unsafe(index, old_label, query), 0); + + ASSERT_EQ(VecSimIndex_RelabelVector(index, old_label, new_label), 1); + + // Nothing was added or removed, and the vector now answers to the new label only. + ASSERT_EQ(VecSimIndex_IndexSize(index), n); + ASSERT_EQ(index->indexLabelCount(), n); + ASSERT_EQ(VecSimIndex_GetDistanceFrom_Unsafe(index, new_label, query), 0); + ASSERT_TRUE(std::isnan(VecSimIndex_GetDistanceFrom_Unsafe(index, old_label, query))); + + auto verify_res = [&](size_t id, double score, size_t rank) { + ASSERT_EQ(id, new_label); + ASSERT_EQ(score, 0); + }; + runTopKSearchTest(index, query, 1, verify_res); + + VecSimIndex_Free(index); +} + +TYPED_TEST(BruteForceTest, relabelVectorRejects) { + size_t dim = 4; + size_t n = 5; + BFParams params = {.dim = dim, .metric = VecSimMetric_L2}; + VecSimIndex *index = this->CreateNewIndex(params); + + for (size_t i = 0; i < n; i++) { + GenerateAndAddVector(index, dim, i, i); + } + + // A missing source, an occupied target and a no-op move are all rejected without modifying + // the index. + ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 100), 0); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 2), 0); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 1), 0); + + ASSERT_EQ(VecSimIndex_IndexSize(index), n); + ASSERT_EQ(index->indexLabelCount(), n); + for (size_t i = 0; i < n; i++) { + TEST_DATA_T v[dim]; + GenerateVector(v, dim, i); + ASSERT_EQ(VecSimIndex_GetDistanceFrom_Unsafe(index, i, v), 0) << "label " << i << " moved"; + } + TEST_DATA_T probe[dim]; + GenerateVector(probe, dim, 0); + ASSERT_TRUE(std::isnan(VecSimIndex_GetDistanceFrom_Unsafe(index, 100, probe))); + + VecSimIndex_Free(index); +} diff --git a/tests/unit/test_bruteforce_multi.cpp b/tests/unit/test_bruteforce_multi.cpp index b93ddcf6e..ed2340744 100644 --- a/tests/unit/test_bruteforce_multi.cpp +++ b/tests/unit/test_bruteforce_multi.cpp @@ -1323,3 +1323,79 @@ TYPED_TEST(BruteForceMultiTest, rangeQuery) { VecSimIndex_Free(index); } + +TYPED_TEST(BruteForceMultiTest, relabelVector) { + size_t dim = 4; + size_t per_label = 3; + BFParams params = {.dim = dim, .metric = VecSimMetric_L2}; + VecSimIndex *index = this->CreateNewIndex(params); + auto *bf_index = this->CastToBF_Multi(index); + + // Two labels, each holding several vectors. + for (size_t i = 0; i < per_label; i++) { + GenerateAndAddVector(index, dim, 0, i); + GenerateAndAddVector(index, dim, 1, i + 10); + } + ASSERT_EQ(VecSimIndex_IndexSize(index), per_label * 2); + ASSERT_EQ(index->indexLabelCount(), 2); + + std::vector> before; + bf_index->getDataByLabel(0, before); + ASSERT_EQ(before.size(), per_label); + + ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 100), 1); + + // Every vector under the label moves together, keeping its data and order, and the untouched + // label is unaffected. + ASSERT_EQ(VecSimIndex_IndexSize(index), per_label * 2); + ASSERT_EQ(index->indexLabelCount(), 2); + std::vector> after; + bf_index->getDataByLabel(100, after); + ASSERT_EQ(after.size(), per_label); + for (size_t i = 0; i < per_label; i++) { + CompareVectors(before[i].data(), after[i].data(), dim); + } + std::vector> other; + bf_index->getDataByLabel(1, other); + ASSERT_EQ(other.size(), per_label); + + // The old label is gone and the new one is searchable. + TEST_DATA_T query[dim]; + GenerateVector(query, dim, 0); + ASSERT_TRUE(std::isnan(VecSimIndex_GetDistanceFrom_Unsafe(index, 0, query))); + ASSERT_EQ(VecSimIndex_GetDistanceFrom_Unsafe(index, 100, query), 0); + auto verify_res = [&](size_t id, double score, size_t rank) { ASSERT_EQ(id, 100); }; + runTopKSearchTest(index, query, 1, verify_res); + + VecSimIndex_Free(index); +} + +TYPED_TEST(BruteForceMultiTest, relabelVectorRejects) { + size_t dim = 4; + size_t per_label = 2; + BFParams params = {.dim = dim, .metric = VecSimMetric_L2}; + VecSimIndex *index = this->CreateNewIndex(params); + auto *bf_index = this->CastToBF_Multi(index); + + for (size_t i = 0; i < per_label; i++) { + GenerateAndAddVector(index, dim, 0, i); + GenerateAndAddVector(index, dim, 1, i + 10); + } + + // A missing source, an occupied target and a no-op move are all rejected. In a multi index an + // accepted move onto an occupied label would silently merge two labels' vectors, so this is the + // case that matters most here. + ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 100), 0); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 1), 0); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 0), 0); + + ASSERT_EQ(VecSimIndex_IndexSize(index), per_label * 2); + ASSERT_EQ(index->indexLabelCount(), 2); + for (labelType label : {0, 1}) { + std::vector> data; + bf_index->getDataByLabel(label, data); + ASSERT_EQ(data.size(), per_label) << "label " << label << " was modified"; + } + + VecSimIndex_Free(index); +} diff --git a/tests/unit/test_svs.cpp b/tests/unit/test_svs.cpp index 764f17316..4cd92094c 100644 --- a/tests/unit/test_svs.cpp +++ b/tests/unit/test_svs.cpp @@ -3459,6 +3459,28 @@ TEST(SVSTest, ThreadPoolLazyInit) { VecSimSVSThreadPoolImpl::instance()->resetForTest(); } +// SVS delegates label management to the external library, so it does not implement relabelVector +// and inherits the VecSimIndexInterface default that reports "unsupported". The source label exists +// and the target is free here, so a 0 return can only come from that default - which is the +// contract callers must handle, and the reason the interface provides a default instead of a pure +// virtual. +TEST(SVSTest, relabelVectorUnsupported) { + 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); + + ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 2), 0); + // The rejected call left the index untouched. + ASSERT_EQ(VecSimIndex_IndexSize(index), 1); + + VecSimIndex_Free(index); +} + #else // HAVE_SVS TEST(SVSTest, svs_not_supported) { From 04caec40c0d16dc3cfdb8995c80a82248ff7f0ad Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Sun, 23 Aug 2026 14:22:27 +0300 Subject: [PATCH 4/5] Report relabelVector outcomes as a status code `relabelVector` returned 1 or 0, so every rejection looked alike: identical labels, a missing source, an occupied target and "this index type never relabels" were all a bare 0. A caller acts on those differently - the first three are conditions it can inspect or resolve, while the last one means it has to fall back to delete + insert - so return a code per outcome instead: `VecSimRelabelCode` with OK / OldLabelMissing / NewLabelTaken / SameLabel / Unsupported, following the existing `VecSimResolveCode` and `VecSimDebugCommandCode` conventions. The tiered index used to combine the three homes of a label with `ret |= ...`, which would have reported OK over a refusal from one of them. Each home is now asked only once it reported holding the label, so - with both guards held and every other rejection ruled out beforehand - a home that is asked can only answer OK; that invariant is asserted, and the outcome is decided by whether the label moved anywhere. Also cover the multi case for the tiered index, where a label has several ids in each of its three homes at once: `relabelVectorMulti` asserts on the whole id set of every home, so a move that handles only the first id of a label - or only one home - fails instead of looking right. Co-Authored-By: Claude Opus 5 (1M context) --- .../brute_force/brute_force_multi.h | 18 ++-- .../brute_force/brute_force_single.h | 18 ++-- src/VecSim/algorithms/hnsw/hnsw.h | 13 +-- .../algorithms/hnsw/hnsw_base_tests_friends.h | 1 + src/VecSim/algorithms/hnsw/hnsw_tiered.h | 37 +++++++-- .../hnsw/hnsw_tiered_tests_friends.h | 1 + src/VecSim/vec_sim.cpp | 3 +- src/VecSim/vec_sim.h | 9 +- src/VecSim/vec_sim_common.h | 12 +++ src/VecSim/vec_sim_interface.h | 18 ++-- tests/benchmark/bm_vecsim_basics.h | 3 +- tests/unit/test_bruteforce.cpp | 8 +- tests/unit/test_bruteforce_multi.cpp | 8 +- tests/unit/test_hnsw.cpp | 12 +-- tests/unit/test_hnsw_multi.cpp | 4 +- tests/unit/test_hnsw_tiered.cpp | 82 +++++++++++++++++-- tests/unit/test_svs.cpp | 2 +- 17 files changed, 182 insertions(+), 67 deletions(-) diff --git a/src/VecSim/algorithms/brute_force/brute_force_multi.h b/src/VecSim/algorithms/brute_force/brute_force_multi.h index ff1b39cdb..2b193e7ae 100644 --- a/src/VecSim/algorithms/brute_force/brute_force_multi.h +++ b/src/VecSim/algorithms/brute_force/brute_force_multi.h @@ -29,7 +29,7 @@ class BruteForceIndex_Multi : public BruteForceIndex { int addVector(const void *vector_data, labelType label) override; int deleteVector(labelType labelType) override; int deleteVectorById(labelType label, idType id) override; - int relabelVector(labelType old_label, labelType new_label) override; + VecSimRelabelCode relabelVector(labelType old_label, labelType new_label) override; double getDistanceFrom_Unsafe(labelType label, const void *vector_data) const override; inline size_t indexLabelCount() const override { return this->labelToIdsLookup.size(); } @@ -223,15 +223,17 @@ int BruteForceIndex_Multi::deleteVectorById(labelType label, } template -int BruteForceIndex_Multi::relabelVector(labelType old_label, - labelType new_label) { +VecSimRelabelCode BruteForceIndex_Multi::relabelVector(labelType old_label, + labelType new_label) { if (old_label == new_label) { - return 0; + return VecSimRelabel_SameLabel; } auto old_it = labelToIdsLookup.find(old_label); - if (old_it == labelToIdsLookup.end() || - labelToIdsLookup.find(new_label) != labelToIdsLookup.end()) { - return 0; + if (old_it == labelToIdsLookup.end()) { + return VecSimRelabel_OldLabelMissing; + } + if (labelToIdsLookup.find(new_label) != labelToIdsLookup.end()) { + return VecSimRelabel_NewLabelTaken; } // Every id under the label moves, so the whole id vector is re-keyed as-is. @@ -242,7 +244,7 @@ int BruteForceIndex_Multi::relabelVector(labelType old_label this->setVectorLabel(id, new_label); } labelToIdsLookup.emplace(new_label, std::move(ids)); - return 1; + return VecSimRelabel_OK; } template diff --git a/src/VecSim/algorithms/brute_force/brute_force_single.h b/src/VecSim/algorithms/brute_force/brute_force_single.h index bae14229c..1519d7e85 100644 --- a/src/VecSim/algorithms/brute_force/brute_force_single.h +++ b/src/VecSim/algorithms/brute_force/brute_force_single.h @@ -27,7 +27,7 @@ class BruteForceIndex_Single : public BruteForceIndex { int addVector(const void *vector_data, labelType label) override; int deleteVector(labelType label) override; int deleteVectorById(labelType label, idType id) override; - int relabelVector(labelType old_label, labelType new_label) override; + VecSimRelabelCode relabelVector(labelType old_label, labelType new_label) override; double getDistanceFrom_Unsafe(labelType label, const void *vector_data) const override; std::unique_ptr @@ -199,15 +199,17 @@ int BruteForceIndex_Single::deleteVectorById(labelType label } template -int BruteForceIndex_Single::relabelVector(labelType old_label, - labelType new_label) { +VecSimRelabelCode BruteForceIndex_Single::relabelVector(labelType old_label, + labelType new_label) { if (old_label == new_label) { - return 0; + return VecSimRelabel_SameLabel; } auto old_it = labelToIdLookup.find(old_label); - if (old_it == labelToIdLookup.end() || - labelToIdLookup.find(new_label) != labelToIdLookup.end()) { - return 0; + if (old_it == labelToIdLookup.end()) { + return VecSimRelabel_OldLabelMissing; + } + if (labelToIdLookup.find(new_label) != labelToIdLookup.end()) { + return VecSimRelabel_NewLabelTaken; } const idType id = old_it->second; @@ -215,7 +217,7 @@ int BruteForceIndex_Single::relabelVector(labelType old_labe labelToIdLookup.emplace(new_label, id); // Keep the id->label direction in sync; `topKQuery` reports results through it. this->setVectorLabel(id, new_label); - return 1; + return VecSimRelabel_OK; } template diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 44801c20a..ec897fb15 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -323,7 +323,7 @@ class HNSWIndex : public VecSimIndexAbstract, // element was marked deleted is *not* considered to exist, matching `getElementIds`. virtual bool isLabelExists(labelType label) = 0; - int relabelVector(labelType old_label, labelType new_label) override; + VecSimRelabelCode relabelVector(labelType old_label, labelType new_label) override; #ifdef BUILD_TESTS void fitMemory() override { @@ -495,21 +495,22 @@ void HNSWIndex::unmarkInProcess(idType internalId) { * consistent with the main-guard-then-data-guard order used by `insertVectorToHNSW`. */ template -int HNSWIndex::relabelVector(labelType old_label, labelType new_label) { +VecSimRelabelCode HNSWIndex::relabelVector(labelType old_label, + labelType new_label) { if (old_label == new_label) { - return 0; + return VecSimRelabel_SameLabel; } std::unique_lock index_data_lock(indexDataGuard); if (isLabelExists(new_label)) { - return 0; + return VecSimRelabel_NewLabelTaken; } // Elements that were marked deleted are already out of the label lookup, so they are reported // as absent here and are left alone - their `idToMetaData` label is still needed by the // pending swap/repair jobs that reference their id. auto ids = getElementIds(old_label); if (ids.empty()) { - return 0; + return VecSimRelabel_OldLabelMissing; } removeLabel(old_label); @@ -519,7 +520,7 @@ int HNSWIndex::relabelVector(labelType old_label, labelType idToMetaData[id].label = new_label; setVectorId(new_label, id); } - return 1; + return VecSimRelabel_OK; } template diff --git a/src/VecSim/algorithms/hnsw/hnsw_base_tests_friends.h b/src/VecSim/algorithms/hnsw/hnsw_base_tests_friends.h index 25c33845a..ada94f643 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_base_tests_friends.h +++ b/src/VecSim/algorithms/hnsw/hnsw_base_tests_friends.h @@ -30,5 +30,6 @@ INDEX_TEST_FRIEND_CLASS(HNSWTest_relabelVectorMarkedDeleted_Test) INDEX_TEST_FRIEND_CLASS(HNSWMultiTest_relabelVectorMulti_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorFlatOnly_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorBothTiers_Test) +INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorMulti_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorRejects_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorDuringIngestion_Test) diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index 9574cbd03..e4113d2cd 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -213,7 +213,7 @@ class TieredHNSWIndex : public VecSimTieredIndex { int addVector(const void *blob, labelType label) override; int deleteVector(labelType label) override; - int relabelVector(labelType old_label, labelType new_label) override; + VecSimRelabelCode relabelVector(labelType old_label, labelType new_label) override; size_t getNumMarkedDeleted() const override { return this->getHNSWIndex()->getNumMarkedDeleted(); } @@ -934,25 +934,37 @@ int TieredHNSWIndex::deleteVector(labelType label) { * `label` field can be unaligned and its store is not atomic. */ template -int TieredHNSWIndex::relabelVector(labelType old_label, labelType new_label) { +VecSimRelabelCode TieredHNSWIndex::relabelVector(labelType old_label, + labelType new_label) { if (old_label == new_label) { - return 0; + return VecSimRelabel_SameLabel; } this->flatIndexGuard.lock(); this->lockMainIndexGuard(); auto *hnsw_index = this->getHNSWIndex(); - int ret = 0; // Reject if the target is taken anywhere, so that a partially applied move is impossible: a // free target in all three homes means none of the updates below can collide. const bool target_taken = this->frontendIndex->isLabelExists(new_label) || this->labelToInsertJobs.find(new_label) != this->labelToInsertJobs.end() || hnsw_index->isLabelExists(new_label); + // The label can live in any subset of the three homes, so the move succeeds if it happened in + // at least one of them. Each home is asked only once it reported holding the label, and the + // check above ruled out every other rejection - the target is free everywhere, and the labels + // differ - so while both guards are held a home that is asked can only answer OK. The asserts + // below state that; a refusal would mean the state changed underneath us. + bool moved = false; if (!target_taken) { if (this->frontendIndex->isLabelExists(old_label)) { - ret |= this->frontendIndex->relabelVector(old_label, new_label); + const VecSimRelabelCode flat_ret = + this->frontendIndex->relabelVector(old_label, new_label); +#ifdef BUILD_TESTS + assert(flat_ret == VecSimRelabel_OK && + "the flat buffer just reported holding this label"); +#endif + moved |= flat_ret == VecSimRelabel_OK; } // Re-key the pending insert jobs *and* rewrite each job's own copy of the label. Both must @@ -967,16 +979,27 @@ int TieredHNSWIndex::relabelVector(labelType old_label, labe job->label = new_label; } this->labelToInsertJobs.emplace(new_label, std::move(jobs)); + moved = true; } // `relabelVector` takes the HNSW index data guard internally, which is the same // main-guard-then-data-guard order that `insertVectorToHNSW` uses. - ret |= hnsw_index->relabelVector(old_label, new_label); + if (hnsw_index->isLabelExists(old_label)) { + const VecSimRelabelCode hnsw_ret = hnsw_index->relabelVector(old_label, new_label); +#ifdef BUILD_TESTS + assert(hnsw_ret == VecSimRelabel_OK && "HNSW just reported holding this label"); +#endif + moved |= hnsw_ret == VecSimRelabel_OK; + } } this->unlockMainIndexGuard(); this->flatIndexGuard.unlock(); - return ret; + + if (target_taken) { + return VecSimRelabel_NewLabelTaken; + } + return moved ? VecSimRelabel_OK : VecSimRelabel_OldLabelMissing; } // `getDistanceFrom` returns the minimum distance between the given blob and the vector with the diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h b/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h index 1b8dcebb0..5e9316a0f 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered_tests_friends.h @@ -79,5 +79,6 @@ INDEX_TEST_FRIEND_CLASS(BM_VecSimCommon) friend class BM_IncomingEdgesBase; INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorFlatOnly_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorBothTiers_Test) +INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorMulti_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorRejects_Test) INDEX_TEST_FRIEND_CLASS(HNSWTieredIndexTestBasic_relabelVectorDuringIngestion_Test) diff --git a/src/VecSim/vec_sim.cpp b/src/VecSim/vec_sim.cpp index 27794c1de..4883239e7 100644 --- a/src/VecSim/vec_sim.cpp +++ b/src/VecSim/vec_sim.cpp @@ -230,7 +230,8 @@ extern "C" int VecSimIndex_DeleteVector(VecSimIndex *index, size_t label) { return index->deleteVector(label); } -extern "C" int VecSimIndex_RelabelVector(VecSimIndex *index, size_t old_label, size_t new_label) { +extern "C" VecSimRelabelCode VecSimIndex_RelabelVector(VecSimIndex *index, size_t old_label, + size_t new_label) { return index->relabelVector(old_label, new_label); } diff --git a/src/VecSim/vec_sim.h b/src/VecSim/vec_sim.h index 72157039a..2895d47cf 100644 --- a/src/VecSim/vec_sim.h +++ b/src/VecSim/vec_sim.h @@ -81,15 +81,16 @@ int VecSimIndex_DeleteVector(VecSimIndex *index, size_t label); * while the vector did not, as a cheap replacement for delete-then-add. * * The move is rejected, leaving the index untouched, if `old_label` does not exist, if `new_label` - * is already in use, or if the labels are equal. Not all index types support this; unsupported - * types report 0. + * is already in use, or if the labels are equal - each with its own code. Not all index types + * support this; those report `VecSimRelabel_Unsupported`, which a caller has to serve by delete + + * insert rather than treat as a no-op. * * @param index the index holding the vector(s). * @param old_label the label currently holding the vector(s). * @param new_label the label to move them to. - * @return 1 if the label was moved, 0 otherwise. + * @return `VecSimRelabel_OK` if the label was moved, otherwise the reason it was not. */ -int VecSimIndex_RelabelVector(VecSimIndex *index, size_t old_label, size_t new_label); +VecSimRelabelCode VecSimIndex_RelabelVector(VecSimIndex *index, size_t old_label, size_t new_label); /** * @brief Calculate the distance of a vector from an index to a vector. This function assumes that diff --git a/src/VecSim/vec_sim_common.h b/src/VecSim/vec_sim_common.h index 63e774a57..3ec945a01 100644 --- a/src/VecSim/vec_sim_common.h +++ b/src/VecSim/vec_sim_common.h @@ -130,6 +130,18 @@ typedef enum { VecSimDebugCommandCode_MultiNotSupported } VecSimDebugCommandCode; +// Outcome of `relabelVector`. The rejections are reported separately because a caller acts on them +// differently: `OldLabelMissing` and `SameLabel` mean there is nothing to do, `NewLabelTaken` is a +// caller-side conflict to resolve, and `Unsupported` says this index type never relabels - so the +// caller has to fall back to delete + insert rather than treat it as a no-op. +typedef enum { + VecSimRelabel_OK = VecSim_OK, // for returning VecSim_OK as an enum value + VecSimRelabel_OldLabelMissing, // `old_label` is not in the index + VecSimRelabel_NewLabelTaken, // `new_label` is already in the index + VecSimRelabel_SameLabel, // `old_label` and `new_label` are equal + VecSimRelabel_Unsupported // this index type does not implement relabeling +} VecSimRelabelCode; + typedef struct AsyncJob AsyncJob; // forward declaration. // Write async/sync mode diff --git a/src/VecSim/vec_sim_interface.h b/src/VecSim/vec_sim_interface.h index 49a637920..873154d6b 100644 --- a/src/VecSim/vec_sim_interface.h +++ b/src/VecSim/vec_sim_interface.h @@ -63,19 +63,23 @@ struct VecSimIndexInterface : public VecsimBaseObject { * alternative to delete-then-add for callers whose external id changed while the vector * itself did not. * - * The operation is rejected (returns 0, index left untouched) if `old_label` is not present, - * if `new_label` is already taken, or if the two labels are equal. Rejecting rather than + * The operation is rejected - index left untouched - if `old_label` is not present, if + * `new_label` is already taken, or if the two labels are equal. Rejecting rather than * overwriting keeps the caller in control: overwriting `new_label` would silently drop a - * vector, and unlike `addVector` there is no replacement data to justify it. + * vector, and unlike `addVector` there is no replacement data to justify it. Each rejection has + * its own code, so a caller can tell a conflict it may resolve from an index type that will + * never relabel and has to be served by delete + insert instead. * - * The default implementation reports "unsupported" so that index types which delegate label - * management to an external library are not forced to implement it. + * The default implementation reports `VecSimRelabel_Unsupported` so that index types which + * delegate label management to an external library are not forced to implement it. * * @param old_label the label currently holding the vector(s). * @param new_label the label to move them to. - * @return 1 if the label was moved, 0 otherwise. + * @return `VecSimRelabel_OK` if the label was moved, otherwise the reason it was not. */ - virtual int relabelVector(labelType old_label, labelType new_label) { return 0; } + virtual VecSimRelabelCode relabelVector(labelType old_label, labelType new_label) { + return VecSimRelabel_Unsupported; + } /** * @brief Calculate the distance of a vector from an index to a vector. diff --git a/tests/benchmark/bm_vecsim_basics.h b/tests/benchmark/bm_vecsim_basics.h index 2405e69b8..da5a6de42 100644 --- a/tests/benchmark/bm_vecsim_basics.h +++ b/tests/benchmark/bm_vecsim_basics.h @@ -287,7 +287,8 @@ void BM_VecSimBasics::RelabelLabel(benchmark::State &st) { size_t attempted = 0; size_t moved = 0; for (auto _ : st) { - moved += VecSimIndex_RelabelVector(index, attempted, target_base + attempted); + moved += VecSimIndex_RelabelVector(index, attempted, target_base + attempted) == + VecSimRelabel_OK; attempted++; } diff --git a/tests/unit/test_bruteforce.cpp b/tests/unit/test_bruteforce.cpp index 021ec6bb2..99f8235a6 100644 --- a/tests/unit/test_bruteforce.cpp +++ b/tests/unit/test_bruteforce.cpp @@ -1692,7 +1692,7 @@ TYPED_TEST(BruteForceTest, relabelVector) { // A relabel must not move the stored data, so the distance from the label's own vector stays 0. ASSERT_EQ(VecSimIndex_GetDistanceFrom_Unsafe(index, old_label, query), 0); - ASSERT_EQ(VecSimIndex_RelabelVector(index, old_label, new_label), 1); + ASSERT_EQ(VecSimIndex_RelabelVector(index, old_label, new_label), VecSimRelabel_OK); // Nothing was added or removed, and the vector now answers to the new label only. ASSERT_EQ(VecSimIndex_IndexSize(index), n); @@ -1721,9 +1721,9 @@ TYPED_TEST(BruteForceTest, relabelVectorRejects) { // A missing source, an occupied target and a no-op move are all rejected without modifying // the index. - ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 100), 0); - ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 2), 0); - ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 1), 0); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 100), VecSimRelabel_OldLabelMissing); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 2), VecSimRelabel_NewLabelTaken); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 1), VecSimRelabel_SameLabel); ASSERT_EQ(VecSimIndex_IndexSize(index), n); ASSERT_EQ(index->indexLabelCount(), n); diff --git a/tests/unit/test_bruteforce_multi.cpp b/tests/unit/test_bruteforce_multi.cpp index ed2340744..f11af1de2 100644 --- a/tests/unit/test_bruteforce_multi.cpp +++ b/tests/unit/test_bruteforce_multi.cpp @@ -1343,7 +1343,7 @@ TYPED_TEST(BruteForceMultiTest, relabelVector) { bf_index->getDataByLabel(0, before); ASSERT_EQ(before.size(), per_label); - ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 100), 1); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 100), VecSimRelabel_OK); // Every vector under the label moves together, keeping its data and order, and the untouched // label is unaffected. @@ -1385,9 +1385,9 @@ TYPED_TEST(BruteForceMultiTest, relabelVectorRejects) { // A missing source, an occupied target and a no-op move are all rejected. In a multi index an // accepted move onto an occupied label would silently merge two labels' vectors, so this is the // case that matters most here. - ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 100), 0); - ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 1), 0); - ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 0), 0); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 100), VecSimRelabel_OldLabelMissing); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 1), VecSimRelabel_NewLabelTaken); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 0), VecSimRelabel_SameLabel); ASSERT_EQ(VecSimIndex_IndexSize(index), per_label * 2); ASSERT_EQ(index->indexLabelCount(), 2); diff --git a/tests/unit/test_hnsw.cpp b/tests/unit/test_hnsw.cpp index 299e376de..9b5191581 100644 --- a/tests/unit/test_hnsw.cpp +++ b/tests/unit/test_hnsw.cpp @@ -2390,7 +2390,7 @@ TYPED_TEST(HNSWTest, relabelVector) { GenerateVector(query, dim, old_label); const double dist_before = hnsw_index->getDistanceFrom_Unsafe(old_label, query); - ASSERT_EQ(VecSimIndex_RelabelVector(index, old_label, new_label), 1); + ASSERT_EQ(VecSimIndex_RelabelVector(index, old_label, new_label), VecSimRelabel_OK); // Nothing was added or removed. ASSERT_EQ(VecSimIndex_IndexSize(index), n); @@ -2430,9 +2430,9 @@ TYPED_TEST(HNSWTest, relabelVectorRejects) { // A missing source, an occupied target and a no-op move are all rejected, and none of them may // leave the index in a modified state. - ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 100), 0); - ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 2), 0); - ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 1), 0); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 100), VecSimRelabel_OldLabelMissing); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 2), VecSimRelabel_NewLabelTaken); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 1), VecSimRelabel_SameLabel); ASSERT_EQ(VecSimIndex_IndexSize(index), n); ASSERT_EQ(index->indexLabelCount(), n); @@ -2459,12 +2459,12 @@ TYPED_TEST(HNSWTest, relabelVectorMarkedDeleted) { // A marked-deleted element is out of the label lookup, so it is reported as absent and left // alone - its `idToMetaData` label is still needed by the swap/repair jobs holding its id. - ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 100), 0); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 100), VecSimRelabel_OldLabelMissing); ASSERT_EQ(hnsw_index->getExternalLabel(deleted_ids[0]), 0); ASSERT_FALSE(hnsw_index->isLabelExists(100)); // A live label in the same index still relabels fine. - ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 101), 1); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 101), VecSimRelabel_OK); ASSERT_TRUE(hnsw_index->isLabelExists(101)); ASSERT_TRUE(hnsw_index->checkIntegrity().valid_state); diff --git a/tests/unit/test_hnsw_multi.cpp b/tests/unit/test_hnsw_multi.cpp index 963a895dd..c9d96fb1a 100644 --- a/tests/unit/test_hnsw_multi.cpp +++ b/tests/unit/test_hnsw_multi.cpp @@ -1645,7 +1645,7 @@ TYPED_TEST(HNSWMultiTest, relabelVectorMulti) { auto ids_before = hnsw_index->getElementIds(0); ASSERT_EQ(ids_before.size(), per_label); - ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 100), 1); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 100), VecSimRelabel_OK); // Every id under the label moves together, keeping its order, and the untouched label is // unaffected. @@ -1666,7 +1666,7 @@ TYPED_TEST(HNSWMultiTest, relabelVectorMulti) { runTopKSearchTest(index, query, 1, verify_res); // Moving onto an occupied label is rejected without disturbing either label. - ASSERT_EQ(VecSimIndex_RelabelVector(index, 100, 1), 0); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 100, 1), VecSimRelabel_NewLabelTaken); ASSERT_EQ(hnsw_index->getElementIds(100).size(), per_label); ASSERT_EQ(hnsw_index->getElementIds(1).size(), per_label); diff --git a/tests/unit/test_hnsw_tiered.cpp b/tests/unit/test_hnsw_tiered.cpp index ae75a659c..c91a15339 100644 --- a/tests/unit/test_hnsw_tiered.cpp +++ b/tests/unit/test_hnsw_tiered.cpp @@ -4605,7 +4605,7 @@ TYPED_TEST(HNSWTieredIndexTestBasic, relabelVectorFlatOnly) { ASSERT_EQ(tiered_index->labelToInsertJobs.at(7).size(), 1); const idType flat_id = tiered_index->labelToInsertJobs.at(7)[0]->id; - ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 7, 70), 1); + ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 7, 70), VecSimRelabel_OK); // The flat tier moved, and so did both halves of the job bookkeeping: the map key and the // job's own copy of the label. A half-applied move would make the worker below either index @@ -4658,7 +4658,7 @@ TYPED_TEST(HNSWTieredIndexTestBasic, relabelVectorBothTiers) { ASSERT_EQ(frontend_index->indexSize(), 1); ASSERT_EQ(hnsw_index->indexSize(), 1); - ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 7, 70), 1); + ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 7, 70), VecSimRelabel_OK); // Both tiers must move - an implementation treating them as mutually exclusive would leave one // stale copy behind under the old label. @@ -4670,6 +4670,69 @@ TYPED_TEST(HNSWTieredIndexTestBasic, relabelVectorBothTiers) { ASSERT_TRUE(hnsw_index->checkIntegrity().valid_state); } +// The multi case is where a label has more than one id in each of its three homes, so a move that +// handles only the first id of a label - or only one home - still looks right in the single-value +// tests. Assert on the whole id set of every home instead. +TYPED_TEST(HNSWTieredIndexTestBasic, relabelVectorMulti) { + size_t dim = 4; + size_t per_label = 3; + 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 *frontend_index = this->GetFlatIndex(tiered_index); + auto *hnsw_index = this->CastToHNSW(tiered_index); + + // Label 7 gets `per_label` vectors in the flat buffer (each with a pending insert job) and + // `per_label` more directly in HNSW, reproducing the ingestion window for every copy. Label 8 + // is an untouched neighbour with copies of its own, so a move that is too broad shows up too. + TEST_DATA_T vector[dim]; + for (size_t i = 0; i < per_label; i++) { + GenerateAndAddVector(tiered_index, dim, 7, i); + GenerateAndAddVector(tiered_index, dim, 8, i + 100); + GenerateVector(vector, dim, i + 10); + hnsw_index->addVector(vector, 7); + } + ASSERT_EQ(frontend_index->indexSize(), per_label * 2); + ASSERT_EQ(hnsw_index->indexSize(), per_label); + ASSERT_EQ(tiered_index->labelToInsertJobs.at(7).size(), per_label); + auto hnsw_ids_before = hnsw_index->getElementIds(7); + ASSERT_EQ(hnsw_ids_before.size(), per_label); + + ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 7, 70), VecSimRelabel_OK); + + // Nothing was added or dropped along the way. + ASSERT_EQ(frontend_index->indexSize(), per_label * 2); + ASSERT_EQ(hnsw_index->indexSize(), per_label); + ASSERT_EQ(tiered_index->indexLabelCount(), 2); + + // Every home moved, and moved *all* of its ids. + ASSERT_TRUE(frontend_index->isLabelExists(70)); + ASSERT_FALSE(frontend_index->isLabelExists(7)); + ASSERT_TRUE(hnsw_index->isLabelExists(70)); + ASSERT_FALSE(hnsw_index->isLabelExists(7)); + ASSERT_EQ(hnsw_index->getElementIds(70), hnsw_ids_before); + for (idType id : hnsw_ids_before) { + ASSERT_EQ(hnsw_index->getExternalLabel(id), 70); + } + ASSERT_EQ(tiered_index->labelToInsertJobs.count(7), 0); + ASSERT_EQ(tiered_index->labelToInsertJobs.at(70).size(), per_label); + for (HNSWInsertJob *job : tiered_index->labelToInsertJobs.at(70)) { + ASSERT_EQ(job->label, 70); + } + + // The neighbouring label kept all of its own copies. + ASSERT_TRUE(frontend_index->isLabelExists(8)); + ASSERT_EQ(tiered_index->labelToInsertJobs.at(8).size(), per_label); + + // The vectors are still searchable, now reported under the new label. + GenerateVector(vector, dim, 0); + auto verify_res = [&](size_t label, double score, size_t rank) { ASSERT_EQ(label, 70); }; + runTopKSearchTest(tiered_index, vector, 1, verify_res); + ASSERT_TRUE(hnsw_index->checkIntegrity().valid_state); +} + TYPED_TEST(HNSWTieredIndexTestBasic, relabelVectorRejects) { size_t dim = 4; HNSWParams params = { @@ -4688,10 +4751,12 @@ TYPED_TEST(HNSWTieredIndexTestBasic, relabelVectorRejects) { ASSERT_EQ(hnsw_index->indexSize(), 1); ASSERT_EQ(frontend_index->indexSize(), 1); - ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 2, 1), 0); // target lives in HNSW - ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 1, 2), 0); // target lives in flat + jobs - ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 1, 1), 0); // no-op - ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 42, 43), 0); // source absent + ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 2, 1), + VecSimRelabel_NewLabelTaken); // target lives in HNSW + ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 1, 2), + VecSimRelabel_NewLabelTaken); // target lives in flat + jobs + ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 1, 1), VecSimRelabel_SameLabel); + ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 42, 43), VecSimRelabel_OldLabelMissing); // Every rejection left both tiers and the job map untouched. ASSERT_TRUE(hnsw_index->isLabelExists(1)); @@ -4732,10 +4797,11 @@ TYPED_TEST(HNSWTieredIndexTestBasic, relabelVectorDuringIngestion) { for (size_t i = 0; i < n; i++) { GenerateAndAddVector(tiered_index, dim, i, i); if (i > 0) { - relabeled += tiered_index->relabelVector(i - 1, i - 1 + relabel_offset); + relabeled += + tiered_index->relabelVector(i - 1, i - 1 + relabel_offset) == VecSimRelabel_OK; } } - relabeled += tiered_index->relabelVector(n - 1, n - 1 + relabel_offset); + relabeled += tiered_index->relabelVector(n - 1, n - 1 + relabel_offset) == VecSimRelabel_OK; mock_thread_pool.thread_pool_join(); diff --git a/tests/unit/test_svs.cpp b/tests/unit/test_svs.cpp index 4cd92094c..244e0c207 100644 --- a/tests/unit/test_svs.cpp +++ b/tests/unit/test_svs.cpp @@ -3474,7 +3474,7 @@ TEST(SVSTest, relabelVectorUnsupported) { GenerateAndAddVector(index, dim, 1); ASSERT_EQ(VecSimIndex_IndexSize(index), 1); - ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 2), 0); + ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 2), VecSimRelabel_Unsupported); // The rejected call left the index untouched. ASSERT_EQ(VecSimIndex_IndexSize(index), 1); From 1a23d0e17d1b2dfd2dcadc92e535b0d3b19abcca Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Sun, 23 Aug 2026 14:59:19 +0300 Subject: [PATCH 5/5] Report an absent old label ahead of an occupied new one The three implementations disagreed when `old_label` was absent *and* `new_label` was taken: brute force checked the source first and answered `OldLabelMissing`, while HNSW and the tiered index checked the target first and answered `NewLabelTaken`. The codes are not interchangeable - the docs describe a missing source as nothing to do and an occupied target as a conflict for the caller to resolve - so a caller that frees the target on `NewLabelTaken` could drop an unrelated vector for a relabel that had nothing to move. Check the source first everywhere, and in the tiered index across all three homes of a label. Deciding the tiered outcome up front also removes the need to track whether the label moved: with the label present, the target free everywhere and both guards held, every home that holds it moves it. Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/algorithms/hnsw/hnsw.h | 14 ++++++---- src/VecSim/algorithms/hnsw/hnsw_tiered.h | 35 +++++++++++++++--------- tests/unit/test_bruteforce.cpp | 3 ++ tests/unit/test_bruteforce_multi.cpp | 3 ++ tests/unit/test_hnsw.cpp | 3 ++ tests/unit/test_hnsw_tiered.cpp | 3 ++ 6 files changed, 42 insertions(+), 19 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index ec897fb15..f78b678c9 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -502,16 +502,18 @@ VecSimRelabelCode HNSWIndex::relabelVector(labelType old_lab } std::unique_lock index_data_lock(indexDataGuard); - if (isLabelExists(new_label)) { - return VecSimRelabel_NewLabelTaken; - } - // Elements that were marked deleted are already out of the label lookup, so they are reported - // as absent here and are left alone - their `idToMetaData` label is still needed by the - // pending swap/repair jobs that reference their id. + // An absent source is reported before an occupied target, so that a caller which resolves + // `NewLabelTaken` by freeing the target is never told to do so for a move that has nothing to + // move. Elements that were marked deleted are already out of the label lookup, so they are + // reported as absent here and are left alone - their `idToMetaData` label is still needed by + // the pending swap/repair jobs that reference their id. auto ids = getElementIds(old_label); if (ids.empty()) { return VecSimRelabel_OldLabelMissing; } + if (isLabelExists(new_label)) { + return VecSimRelabel_NewLabelTaken; + } removeLabel(old_label); for (idType id : ids) { diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index e4113d2cd..64a266906 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -944,19 +944,24 @@ VecSimRelabelCode TieredHNSWIndex::relabelVector(labelType o this->lockMainIndexGuard(); auto *hnsw_index = this->getHNSWIndex(); - // Reject if the target is taken anywhere, so that a partially applied move is impossible: a - // free target in all three homes means none of the updates below can collide. + // A label can live in any subset of its three homes, so both questions are asked of all of + // them: it is present if any home holds it, and the target is free only if none does. Reject if + // the target is taken anywhere, so that a partially applied move is impossible - a free target + // in all three homes means none of the updates below can collide. + const bool source_exists = + this->frontendIndex->isLabelExists(old_label) || + this->labelToInsertJobs.find(old_label) != this->labelToInsertJobs.end() || + hnsw_index->isLabelExists(old_label); const bool target_taken = this->frontendIndex->isLabelExists(new_label) || this->labelToInsertJobs.find(new_label) != this->labelToInsertJobs.end() || hnsw_index->isLabelExists(new_label); - // The label can live in any subset of the three homes, so the move succeeds if it happened in - // at least one of them. Each home is asked only once it reported holding the label, and the - // check above ruled out every other rejection - the target is free everywhere, and the labels - // differ - so while both guards are held a home that is asked can only answer OK. The asserts - // below state that; a refusal would mean the state changed underneath us. - bool moved = false; - if (!target_taken) { + + // Each home is asked to move the label only once it reported holding it, and the checks above + // ruled out every other rejection - the label is present, the target is free everywhere, and + // the labels differ - so while both guards are held a home that is asked can only answer OK. + // The asserts below state that; a refusal would mean the state changed underneath us. + if (source_exists && !target_taken) { if (this->frontendIndex->isLabelExists(old_label)) { const VecSimRelabelCode flat_ret = this->frontendIndex->relabelVector(old_label, new_label); @@ -964,7 +969,7 @@ VecSimRelabelCode TieredHNSWIndex::relabelVector(labelType o assert(flat_ret == VecSimRelabel_OK && "the flat buffer just reported holding this label"); #endif - moved |= flat_ret == VecSimRelabel_OK; + UNUSED(flat_ret); } // Re-key the pending insert jobs *and* rewrite each job's own copy of the label. Both must @@ -979,7 +984,6 @@ VecSimRelabelCode TieredHNSWIndex::relabelVector(labelType o job->label = new_label; } this->labelToInsertJobs.emplace(new_label, std::move(jobs)); - moved = true; } // `relabelVector` takes the HNSW index data guard internally, which is the same @@ -989,17 +993,22 @@ VecSimRelabelCode TieredHNSWIndex::relabelVector(labelType o #ifdef BUILD_TESTS assert(hnsw_ret == VecSimRelabel_OK && "HNSW just reported holding this label"); #endif - moved |= hnsw_ret == VecSimRelabel_OK; + UNUSED(hnsw_ret); } } this->unlockMainIndexGuard(); this->flatIndexGuard.unlock(); + // An absent source outranks an occupied target: a caller that resolves `NewLabelTaken` by + // freeing the target would otherwise drop an unrelated vector for a move with nothing to move. + if (!source_exists) { + return VecSimRelabel_OldLabelMissing; + } if (target_taken) { return VecSimRelabel_NewLabelTaken; } - return moved ? VecSimRelabel_OK : VecSimRelabel_OldLabelMissing; + return VecSimRelabel_OK; } // `getDistanceFrom` returns the minimum distance between the given blob and the vector with the diff --git a/tests/unit/test_bruteforce.cpp b/tests/unit/test_bruteforce.cpp index 99f8235a6..367326be0 100644 --- a/tests/unit/test_bruteforce.cpp +++ b/tests/unit/test_bruteforce.cpp @@ -1724,6 +1724,9 @@ TYPED_TEST(BruteForceTest, relabelVectorRejects) { ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 100), VecSimRelabel_OldLabelMissing); ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 2), VecSimRelabel_NewLabelTaken); ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 1), VecSimRelabel_SameLabel); + // An absent source outranks an occupied target: a caller that resolves the conflict by + // freeing the target must not be sent down that path for a move with nothing to move. + ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 1), VecSimRelabel_OldLabelMissing); ASSERT_EQ(VecSimIndex_IndexSize(index), n); ASSERT_EQ(index->indexLabelCount(), n); diff --git a/tests/unit/test_bruteforce_multi.cpp b/tests/unit/test_bruteforce_multi.cpp index f11af1de2..ecf70fa02 100644 --- a/tests/unit/test_bruteforce_multi.cpp +++ b/tests/unit/test_bruteforce_multi.cpp @@ -1388,6 +1388,9 @@ TYPED_TEST(BruteForceMultiTest, relabelVectorRejects) { ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 100), VecSimRelabel_OldLabelMissing); ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 1), VecSimRelabel_NewLabelTaken); ASSERT_EQ(VecSimIndex_RelabelVector(index, 0, 0), VecSimRelabel_SameLabel); + // An absent source outranks an occupied target: a caller that resolves the conflict by + // freeing the target must not be sent down that path for a move with nothing to move. + ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 1), VecSimRelabel_OldLabelMissing); ASSERT_EQ(VecSimIndex_IndexSize(index), per_label * 2); ASSERT_EQ(index->indexLabelCount(), 2); diff --git a/tests/unit/test_hnsw.cpp b/tests/unit/test_hnsw.cpp index 9b5191581..4baf526fa 100644 --- a/tests/unit/test_hnsw.cpp +++ b/tests/unit/test_hnsw.cpp @@ -2433,6 +2433,9 @@ TYPED_TEST(HNSWTest, relabelVectorRejects) { ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 100), VecSimRelabel_OldLabelMissing); ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 2), VecSimRelabel_NewLabelTaken); ASSERT_EQ(VecSimIndex_RelabelVector(index, 1, 1), VecSimRelabel_SameLabel); + // An absent source outranks an occupied target: a caller that resolves the conflict by + // freeing the target must not be sent down that path for a move with nothing to move. + ASSERT_EQ(VecSimIndex_RelabelVector(index, 42, 1), VecSimRelabel_OldLabelMissing); ASSERT_EQ(VecSimIndex_IndexSize(index), n); ASSERT_EQ(index->indexLabelCount(), n); diff --git a/tests/unit/test_hnsw_tiered.cpp b/tests/unit/test_hnsw_tiered.cpp index c91a15339..fc049b34c 100644 --- a/tests/unit/test_hnsw_tiered.cpp +++ b/tests/unit/test_hnsw_tiered.cpp @@ -4757,6 +4757,9 @@ TYPED_TEST(HNSWTieredIndexTestBasic, relabelVectorRejects) { VecSimRelabel_NewLabelTaken); // target lives in flat + jobs ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 1, 1), VecSimRelabel_SameLabel); ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 42, 43), VecSimRelabel_OldLabelMissing); + // An absent source outranks an occupied target: a caller that resolves the conflict by + // freeing the target must not be sent down that path for a move with nothing to move. + ASSERT_EQ(VecSimIndex_RelabelVector(tiered_index, 42, 1), VecSimRelabel_OldLabelMissing); // Every rejection left both tiers and the job map untouched. ASSERT_TRUE(hnsw_index->isLabelExists(1));