[MOD-17688] Add relabelVector API - #1017
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1017 +/- ##
==========================================
+ Coverage 97.18% 97.28% +0.09%
==========================================
Files 141 141
Lines 8537 8621 +84
==========================================
+ Hits 8297 8387 +90
+ Misses 240 234 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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) <noreply@anthropic.com>
Benchmark resultsFrom the
Single (1 vector/label) — arm64 / x86_64
Multi (25 vectors/label) — arm64
Reading
Caveats
🤖 Generated with Claude Code |
ofiryanai
left a comment
There was a problem hiding this comment.
Overall looking good, one comment regarding the API
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit aa417ae. Configure here.
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
aa417ae to
1a23d0e
Compare

Describe the changes in the pull request
Adds
relabelVector(old_label, new_label), which moves 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:
idToMetaData[id].label(read bygetExternalLabelon every query result) and the derived class's label→id lookup. 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
VecSimRelabel_Unsupportedrather than being forced to implement it.Semantics. 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
addVectorthere is no replacement data to justify it. In the tiered index the target must be free in all three homes of a label (flat buffer, pending insert jobs, HNSW), which is what makes a partially applied move impossible.Each outcome has its own code rather than a shared
0, because a caller acts on them differently —OldLabelMissingandSameLabelmean there is nothing to do,NewLabelTakenis a conflict it may resolve, andUnsupportedmeans it has to fall back to delete + insert:This follows the existing
VecSimResolveCode/VecSimDebugCommandCodeconventions, including_OK = VecSim_OKas the first member.The codes are ordered rather than independent: an absent
old_labelis reported ahead of an occupiednew_label, in all three implementations and — for the tiered index — across all three homes of a label. They are not interchangeable, so a caller that resolvesNewLabelTakenby freeing the target must never be sent down that path for a move that had nothing to move.Three details worth a reviewer's attention
HNSW assigns
idToMetaData[id].labelas a field rather than replacing theElementMetaDatastruct, whose constructor resetsflagstoIN_PROCESS. Relabeling an element that is mid-insertion is a reachable state —executeInsertJobpopulates HNSW before removing the vector from the flat buffer — and struct assignment would reset that flag on an element the ingesting worker is about tounmarkInProcess, leaving it permanently invisible to queries.The tiered index moves all four homes of a label under one
flatIndexGuard-exclusive thenmainIndexGuard-exclusive section. That is the flat→main order already used byinsertVectorToHNSWandtopKQueryImp, 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 (unlikemarkDelete, which only touches the atomicflagsbyte): query threads readgetExternalLabelholding only a shared main guard —topKQuerydoes not takeindexDataGuard— andElementMetaDatais#pragma pack(1), so thelabelstore can be unaligned and is not atomic. Downgrading to shared would require unpacking the struct at +7 bytes per element, and would not reduce query blocking anyway, since every read path takes the flat guard first.In the tiered index, each of the three homes is asked to move the label only once it has reported holding it. With both guards held and every other rejection already ruled out — the target is free everywhere, the labels differ — a home that is asked can only answer OK, so a refusal would mean the state changed underneath us; that invariant is asserted (under
BUILD_TESTS) rather than folded into the return value, and the outcome is decided by whether the label moved anywhere. The earlierret |= ...form would have reported OK over a refusal from one home.Critically, the job map key and each pending job's own
labelcopy must move together:executeInsertJobindexes into HNSW underjob->labeland then looks the job up withlabelToInsertJobs.at(job->label), so a half-applied move either indexes the vector under the stale label or throwsstd::out_of_rangeout of a worker thread — which terminates the process rather than failing a command.relabelVectorDuringIngestioncovers that path; removing thejob->labelrewrite makes it abort with exactly that exception.Which issues this PR fixes
Main objects this PR modified
VecSimIndexInterface— new virtual with an "unsupported" default;VecSimIndex_RelabelVectorC APIBruteForceIndex_Single/BruteForceIndex_Multi— re-key the label lookup, keep the id→label direction in sync viasetVectorLabelHNSWIndex— shared implementation over the existing label-lookup primitives, plus a newisLabelExistsprimitive implemented by both single and multiTieredHNSWIndex— moves both tiers plus the pending-insert-job bookkeeping under one critical sectionTesting
14 tests across
test_bruteforce.cpp,test_bruteforce_multi.cpp,test_hnsw.cpp,test_hnsw_multi.cpp,test_hnsw_tiered.cppandtest_svs.cpp: happy path, all three rejection cases, marked-deleted elements, multi (all ids under a label move together), the SVS "unsupported" default, tiered flat-only with a pending job drained afterwards, tiered both-tiers, and a 500-vector concurrent test that races relabels against live ingestion by the worker threads. Every rejection now asserts the specific code, so a wrong reason fails rather than passing as a generic 0 — including the case where the source is absent and the target is taken, which is what pins the ordering between the two codes (verified by restoring the old check order:relabelVectorRejectsfails).relabelVectorMulti(tiered) covers the case where a label has several ids in each of its three homes at once — flat buffer, pending insert jobs and HNSW — and 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. Verified by patchingHNSWIndex::relabelVectorto move onlyids.front(): that test fails, as does the existingHNSWMultiTestone.Full unit suite green;
checkIntegrity()asserted after every relabel, and per-label distance checks confirm no relabel crossed wires.Mark if applicable
Serialization is unaffected:
saveGraph/restoreGraphalready persist the label fromidToMetaDataand rebuild the label lookup from it, so a relabel round-trips with no format change.🤖 Generated with Claude Code
Note
Medium Risk
New concurrent mutation path on core index label maps, including exclusive locking in tiered HNSW while queries read labels. Incorrect bookkeeping could desync pending insert jobs or hide live HNSW elements.
Overview
Adds
VecSimIndex_RelabelVectorso callers can move vector(s) fromold_labeltonew_labelby rewriting label bookkeeping only — no vector copy, no HNSW graph change. Intended as a cheap alternative to delete-then-add when the external id changes.Implemented for brute force (single/multi), HNSW (single/multi), and tiered HNSW. SVS keeps the interface default
VecSimRelabel_Unsupported. Rejections leave the index untouched: missing source, occupied target, or same labels, with missing source reported first. DistinctVecSimRelabelCodevalues let callers distinguish no-op, conflict, and fallback-to-delete+insert.HNSW updates
idToMetaData[id].labelas a field (not whole-struct assign, which would resetIN_PROCESSflags) and re-keys the label lookup. Tiered HNSW moves the flat buffer, pending insert jobs (map key andjob->label), and HNSW under exclusiveflatIndexGuardthen exclusive main guard, so a label that lives in more than one home during ingest cannot be half-moved.Covered by unit tests (happy path, rejects, multi-id, marked-deleted, SVS unsupported, tiered both-tiers and concurrent ingest) plus a microsecond-scale relabel benchmark.
Reviewed by Cursor Bugbot for commit 1a23d0e. Bugbot is set up for automated code reviews on this repo. Configure here.