Skip to content

[MOD-17688] Add a relabelVector API - #1026

Closed
nonirosenfeldredis wants to merge 5 commits into
mainfrom
sharon-17688-relabel
Closed

[MOD-17688] Add a relabelVector API#1026
nonirosenfeldredis wants to merge 5 commits into
mainfrom
sharon-17688-relabel

Conversation

@nonirosenfeldredis

@nonirosenfeldredis nonirosenfeldredis commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Describe the changes in the pull request

Adds VecSimIndex_RelabelVector(index, old_label, new_label), which moves the vector(s) stored
under one label to another without re-inserting them.

The caller is RediSearch (MOD-17688). Updating a document assigns it a new doc-id, and today
every vector it owns is deleted and re-added even when the vector itself did not change — on
HNSW that is a graph insertion plus a tombstone the GC has to collect later. Moving the label
does the same job as bookkeeping.

Outcomes are reported as a status code rather than a boolean, because a caller acts on them
differently: OldLabelMissing and SameLabel mean there is nothing to do, NewLabelTaken is a
conflict the caller may be able to resolve, and Unsupported says this index type never
relabels, so the caller must fall back to delete + insert instead of treating it as a no-op.

Implemented for brute-force (single and multi), HNSW (single and multi), and tiered HNSW. The
tiered case is the involved one: a label can live in the flat buffer, in pending insert jobs, or
in the HNSW graph, so all three are asked before anything moves, and the pending jobs are
re-keyed together with their own copy of the label — executeInsertJob indexes under
job->label and then looks the job up by it, so a half-applied move either indexes under the
stale label or throws out of a worker thread.

Which issues this PR fixes

  1. MOD-17688

Main objects this PR modified

  1. VecSimIndexInterface::relabelVector — new virtual, defaulting to Unsupported
  2. BruteForceIndex_Single/_Multi, HNSWIndex_Single/_Multi — implementations
  3. TieredHNSWIndex::relabelVector — all three homes of a label, under both guards
  4. VecSimRelabelCode — the outcome enum

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

Note on ordering

#1025 is a sibling of this branch, not a descendant — both branch from the same point on main.
Together they are what MOD-17688 needs on the RediSearch side; neither depends on the other to
build.

🤖 Generated with Claude Code


Note

Medium Risk
Tiered relabel touches concurrent ingestion (job map + dual-tier state) under new lock ordering; incorrect partial updates could break workers or query label reporting, though extensive tests mitigate this.

Overview
Adds VecSimIndex_RelabelVector and VecSimRelabelCode so callers can move vector(s) from one label to another by updating label bookkeeping only—no vector copy and, for HNSW, no graph changes. This is meant as a cheap alternative to delete-then-add when an external id changes (e.g. RediSearch MOD-17688).

API: New C entry point and VecSimIndexInterface::relabelVector with a default of VecSimRelabel_Unsupported for index types that do not implement it (e.g. SVS). Distinct codes cover missing source, occupied target, same label, success, and unsupported so callers can treat conflicts vs no-ops vs fallback to delete+insert.

Implementations: Brute-force single/multi re-key label maps and sync id→label. HNSW updates idToMetaData[].label and the label lookup under indexDataGuard, skips marked-deleted labels, and adds isLabelExists. Tiered HNSW coordinates flat buffer, labelToInsertJobs (map key and job->label), and HNSW under flat-then-main exclusive locks, with source/target checks across all three homes before any move.

Tests & benchmarks: Unit coverage for BF/HNSW/tiered (including ingestion races and reject ordering); microbenchmark RelabelLabel for BF, HNSW, and tiered indexes.

Reviewed by Cursor Bugbot for commit 1a23d0e. Bugbot is set up for automated code reviews on this repo. Configure here.

nonirosenfeldredis and others added 5 commits August 24, 2026 13:02
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>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ 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 1a23d0e. Configure here.

// resets `flags` to IN_PROCESS, which would hide a live element from queries.
idToMetaData[id].label = new_label;
setVectorId(new_label, id);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HNSW relabel races with queries

Medium Severity

HNSWIndex::relabelVector writes idToMetaData[id].label under only indexDataGuard, but searches call getExternalLabel without that lock. ElementMetaData is #pragma pack(1), so the label store is not atomic. Concurrent search can observe a torn or stale label. Tiered relabel avoids this with an exclusive main guard; standalone HNSW does not.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1a23d0e. Configure here.

@nonirosenfeldredis

Copy link
Copy Markdown
Collaborator Author

Closing as a duplicate: this is the same work that landed in #1017, which is already merged to main.

I opened this in error. I had checked git branch -r --contains against a stale origin/main — the fetch had failed silently on a proxy — so the series looked unpushed when it was in fact upstream. Verified now: every relabel file (vec_sim_interface.h, hnsw_tiered.h, hnsw_single.h, brute_force_single.h, vec_sim_common.h) is byte-identical to main, and #1017 also carries the RelabelLabel benchmark and the brute-force / SVS coverage tests from this branch, so nothing here needs salvaging. git cherry still listed the commits as unmerged only because #1017 landed squashed.

#1025 now merges current main, so it carries both halves of what MOD-17688 needs on the RediSearch side.

@nonirosenfeldredis
nonirosenfeldredis deleted the sharon-17688-relabel branch August 27, 2026 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant