[MOD-17706] finalize SVSIndex::relabelVector - #1045
Open
nonirosenfeldredis wants to merge 2 commits into
Open
Conversation
SVS delegates label management to its library, so relabeling was previously reported as unsupported. The library now offers `replace_external_id`, which renames an external id (single) or a label (multi) purely in the id translation table, leaving the dataset and the graph alone -- exactly the guarantee the in-tree indexes give. Bump the submodule to a7e3494, which adds it, and route relabelVector through it. Preconditions are checked here rather than left to the library: it throws on a bad one, and this API answers with a code. Checking `has_id` for both labels first also means the throwing validation inside `replace_external_id` cannot fire, so no exception escapes into the C API. `markIndexUpdate` is deliberately not called - nothing was added or deleted and the label count is unchanged, so the index owes no consolidation. Gated on availability, following the pattern svs.cmake already uses for LVQ. This matters because SVS_SHARED_LIB (Linux x86_64) downloads a pre-built SVS release rather than building the submodule, and none of v0.3.0/v0.3.1/v0.3.2 carries `replace_external_id`. LVQ tests for a header's existence; this is a method, so the check greps that header instead. Where it is missing the override is left out entirely and the interface default reports `VecSimRelabel_Unsupported`, which tells a caller to fall back to delete + insert rather than read it as a no-op. The unit tests are gated the same way, asserting the real behavior where the API exists and the unsupported code where it does not, so the contract stays covered in both configurations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two changes to the bindings, both needed before any of this could be tested from python: `relabel_vector` had no binding at all, so no flow test could reach it. Bind it on PyVecSimIndex, which serves every index type, and expose VecSimRelabelCode so a caller can tell the rejections apart rather than compare bare ints. `get_vector` was already bound, but its dispatch treated "not brute force" as "is HNSW". SVSIndex derives from VecSimIndexAbstract, not HNSWIndex, so the dynamic_cast returned nullptr and the virtual call dereferenced it - `svs_index.get_vector(label)` was a segfault, which is why the SVS getDataByLabel merged in #1033 was unreachable from python. `getDataByLabel` is a pure virtual on VecSimIndexAbstract, so the algo switch was never needed: call it virtually and brute force, HNSW and SVS are all served. A tiered index wraps two VecSimIndexAbstract instances rather than being one, so it keeps its own branch - that fixes the same latent null dereference for tiered, which was pre-existing and unrelated to SVS. Flow tests cover both APIs, single and multi, across every index type. Multi is not a formality here: a label owns several vectors and each backend moves them by a different route, and a move onto an occupied label must be rejected because accepting it would silently merge two labels' vectors rather than lose one. The tiered test relabels an early label and the last-inserted one. Workers ingest in insertion order, so the early one is already in HNSW while the late one is still buffered with pending ingest jobs - measured at ~690 of 1000 still buffered at relabel time - so both tiers get covered. The buffered case is the delicate one: a job left holding the old label would either ingest under it or throw out of a worker thread. The SVS flow tests probe the build with a no-op relabel and skip when it reports unsupported, since python cannot see HAVE_SVS_REPLACE_EXTERNAL_ID. The probe uses equal labels, which is answered before `impl_` is touched, so it works on an empty index and cannot mask a broken implementation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nonirosenfeldredis
marked this pull request as ready for review
September 9, 2026 09:15
nonirosenfeldredis
enabled auto-merge
September 9, 2026 09:42
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1045 +/- ##
=======================================
Coverage 97.40% 97.40%
=======================================
Files 141 141
Lines 8698 8698
=======================================
Hits 8472 8472
Misses 226 226 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the changes in the pull request
SVS delegates label management to its library, so
relabelVectorwas reported as unsupported. The library now offersreplace_external_id, which renames an external id (single) or a label (multi) purely in the id translation table — leaving the dataset and graph untouched, which is the same guarantee the in-tree indexes give. This bumps the submodule toa7e3494and routesrelabelVectorthrough it.Preconditions are checked in
svs.hrather than left to the library: it throws on a bad one, and this API answers with a code. Checkinghas_idfor both labels first also means the throwing validation insidereplace_external_idcannot fire, so no exception escapes into the C API.markIndexUpdateis deliberately not called — nothing was added or deleted and the label count is unchanged, so no consolidation is owed.Availability gate.
SVS_SHARED_LIB(Linux x86_64) downloads a pre-built SVS release rather than building the submodule, andreplace_external_idis in none of v0.3.0 / v0.3.1 / v0.3.2 — nor is v0.3.2 an ancestor of SVSmain. So availability depends on which SVS a build picked up, not on the platform.cmake/svs.cmakenow detects it following the pattern already used for LVQ, with one necessary difference: LVQ tests for a header file's existence, but this is a method, so the check greps that header's contents. Where it is missing, the override is left out entirely and the interface default reportsVecSimRelabel_Unsupported— telling a caller to fall back to delete + insert rather than read it as a no-op.Python bindings. Two changes, both prerequisites for testing any of this from python:
relabel_vectorhad no binding, so no flow test could reach it. Bound onPyVecSimIndex(serves every index type), withVecSimRelabelCodeexposed so callers can tell the rejections apart.get_vectorwas bound, but its dispatch treated "not brute force" as "is HNSW".SVSIndexderives fromVecSimIndexAbstract, notHNSWIndex, so thedynamic_castreturnednullptrand the virtual call dereferenced it —svs_index.get_vector(label)was a segfault, which is why the SVSgetDataByLabelmerged in Implement SVSIndex::getDataByLabel (MOD-17706) #1033 was unreachable from python. SincegetDataByLabelis a pure virtual onVecSimIndexAbstract, the algo switch was never needed: call it virtually and BF, HNSW and SVS are all served. Tiered keeps its own branch because it wraps twoVecSimIndexAbstractinstances rather than being one.Which issues this PR fixes
Main objects this PR modified
SVSIndex::relabelVector— viaimpl_->replace_external_id, gated on availabilitycmake/svs.cmake—HAVE_SVS_REPLACE_EXTERNAL_IDdetectionPyVecSimIndex—relabel_vectorbinding,VecSimRelabelCodeenum,getDataByLabeldispatch fixTesting
Unit tests are gated the same way as the implementation, asserting real behavior where the API exists and the unsupported code where it does not, so the contract stays covered in both configurations. Flow tests cover both APIs, single and multi, across every index type.
Multi is not a formality: a label owns several vectors, each backend moves them by a different route, and a move onto an occupied label must be rejected because accepting it would silently merge two labels' vectors rather than lose one.
The tiered test relabels an early label and the last-inserted one. Workers ingest in insertion order, so the early one is already in HNSW while the late one is still buffered with pending ingest jobs — measured at ~690 of 1000 still buffered at relabel time — so both tiers are covered. The buffered case is the delicate one: a job left holding the old label would either ingest under it or throw out of a worker thread.
Both sides of the gate are verified, using real SVS checkouts rather than a forced macro:
a7e3494(has API)replace_external_id found - SVS relabeling enabled, macro=1relabelVector,relabelVectorRejects— pass7786d43(v0.3.0, no API)replace_external_id not found - reports unsupported, macro=0relabelVectorUnsupported— passesSo the same source tree configures and builds correctly against either SVS, and a build without the API reports
VecSimRelabel_Unsupportedrather than failing to compile.Mark if applicable
Follow-up, not blocking this PR
SVS relabeling stays
Unsupportedon Linux x86_64 until an SVS release carries intel/ScalableVectorSearch#383 andSVS_URLincmake/svs.cmakeis bumped to it. That is an external dependency; the gate makes the code correct and compiling in the meantime, which is what the table above verifies.Notes for the reviewer
get_vectordispatch fix is a pre-existing bug unrelated to SVS relabel — it fell out of doing the fix properly rather than special-casing SVS.add_compile_definitions(VectorSimilarity PUBLIC ...)call copies the existing LVQ block verbatim for consistency. Worth knowing that form also defines bareVectorSimilarityandPUBLICmacros (visible inCOMPILE_DEFS), sinceadd_compile_definitionstakes definitions rather than a target — pre-existing, and I did not change it.make flow_testcannot configure with CMake 4.x because pybind11 is pinned at v2.10.1, andpoetry installneedssetuptoolsin the venv on Python 3.13+.🤖 Generated with Claude Code
Note
Medium Risk
Touches index label identity across SVS, tiered buffers, and Python
get_vectordispatch; behavior is gated on SVS version but relabel mistakes could corrupt label→vector mapping.Overview
Adds SVS index label relabeling by wiring
SVSIndex::relabelVectorto the library’sreplace_external_idwhen the linked SVS build exposes it, with explicitVecSimRelabel_*outcomes for same label, missing old label, and taken new label.Build-time availability is detected in
cmake/svs.cmake(header grep forreplace_external_id, not just file presence) and exposed asHAVE_SVS_REPLACE_EXTERNAL_ID; older prebuilt SVS binaries omit the override so callers still getVecSimRelabel_Unsupported.Python bindings expose
relabel_vectorandVecSimRelabelCode, and fixget_vectorto callgetDataByLabelviaVecSimIndexAbstract(plus a tiered branch) so SVS—and tiered—no longer crash on the old BF-vs-HNSW cast.Tests add flow coverage for relabel (single/multi) on brute force, HNSW, and tiered HNSW; SVS flow tests for
get_vectorand relabel (skip when unsupported); unit tests split on the same CMake gate.Reviewed by Cursor Bugbot for commit a68d357. Bugbot is set up for automated code reviews on this repo. Configure here.