Let production code read a label's stored vectors by nonirosenfeldredis · Pull Request #1025 · RedisAI/VectorSimilarity

nonirosenfeldredis · 2026-08-26T11:18:51Z

Describe the changes in the pull request

Makes getDataByLabel usable outside BUILD_TESTS, so production code can ask what a
label is holding rather than only how far another vector is from it.

The motivating caller is RediSearch (MOD-17688). When a document is updated it gets a new
doc-id, and today every vector it owns is deleted and re-added even when the vector itself
did not change. Comparing the value about to be written against the stored one lets the
existing entry be moved to the new doc-id instead. A distance cannot serve that comparison:
VecSimIndex_GetDistanceFrom_Unsafe returns 0 for distinct vectors under IP, and under
cosine it requires a pre-normalized query, leaving only a tolerance — which calls two
nearby-but-different vectors equal. Byte equality of the stored form is the sound test, and
that needs the accessor.

Two fixes come with it, both consequences of the accessor now having non-test callers:

Asking about an absent label was unsafe. The single-value implementations used
labelToIdLookup.at(), which throws; the multi-value ones dereferenced find() without
comparing to end(), which is undefined behaviour. Both now leave the output empty, so
the output size answers whether the label is held.
A tiered index reported nothing for a label still in the flat buffer.
TieredHNSWIndex::getDataByLabel consulted the backend alone, so everything written since
the last ingest job looked absent — worst for recently written vectors, which are the ones
most likely to be written again. The lookup now asks the frontend first, under the guards
relabelVector takes and in the same order. Nothing about it is HNSW-specific, so it moved
to VecSimTieredIndex, where the SVS tiered index gets it too.

The base declaration becomes a defaulted virtual rather than pure, so SVS — whose
implementation is a not-implemented stub — needs no production definition, and an empty
output means "cannot tell" exactly as an absent label does.

Which issues this PR fixes

MOD-17688

Main objects this PR modified

VecSimIndexAbstract::getDataByLabel — out of BUILD_TESTS, defaulted rather than pure
BruteForceIndex_Single/_Multi, HNSWIndex_Single/_Multi — same, and absence-safe
VecSimTieredIndex::getDataByLabel — new, asks the tier holding the label
TieredHNSWIndex::getDataByLabel — removed, the base covers it

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes

Testing

ctest on this branch: 2759/2759 passed, including the 97 tiered HNSW tests. No test called
the tiered wrapper's getDataByLabel before this change — the seven existing uses all call a
tier directly — so the changed tiered behaviour is not covered yet; the RediSearch side that
motivates it is what exercises it today.

🤖 Generated with Claude Code

Note

Medium Risk
Changes tiered read locking/semantics and a new production API used for document update paths; behavior is intentional but affects concurrency and equality decisions for quantized/SVS indexes.

Overview
Promotes getDataByLabel from test-only to a documented production API on VecSimIndexAbstract, so callers (notably RediSearch MOD-17688) can compare stored vector bytes on update instead of relying on distance.

Index implementations are hardened for real use: missing labels leave the output empty instead of throwing or UB; quantized indexes (SQ8/HNSW) return nothing rather than memcpy-ing compressed layout as floats, keyed off isQuantized not blob size. HNSW reads take indexDataGuard under the same rules as ingest/delete.

Tiered lookup is centralized in VecSimTieredIndex: read the flat buffer first, then the backend when multi-value or the label was not in the buffer—fixing “absent” for vectors still buffered. SVS backends skip reads entirely (partial tier answers would lie) until MOD-17706; SVSIndex::getDataByLabel logs and returns empty instead of asserting. TieredHNSWIndex drops its duplicate wrapper.

Unit tests cover absent labels, SQ8 at dim 4 and 40, tiered HNSW spanning both tiers, and tiered SVS reporting nothing.

^{Reviewed by Cursor Bugbot for commit aaeafd0. Bugbot is set up for automated code reviews on this repo. Configure here.}

`getDataByLabel` was declared and implemented inside `BUILD_TESTS`, so the only way production code could learn anything about a stored vector was its distance from another one -- which is not an equality test. Comparing a vector about to be written against the one already stored lets a caller replacing a document skip re-adding an unchanged vector, so the accessor moves out of the test-only guard. The base declaration becomes a defaulted virtual rather than pure: an index type that cannot hand its vectors back (SVS, whose implementation is a not-implemented stub) then needs no production definition, and an empty output says "cannot tell" in the same way an absent label does. That contract also removes two ways of asking about a label that does not exist. The single-value implementations called `labelToIdLookup.at()`, which throws, and the multi-value ones dereferenced `find()` without comparing against `end()`, which is undefined behaviour. Both now leave the output empty, which is what makes the output size usable as the answer to whether the index holds the label. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`TieredHNSWIndex::getDataByLabel` delegated to the backend index alone, so a label still sitting in the flat buffer -- everything written since the last ingest job -- reported nothing. That is the wrong answer for a caller asking what a label holds, and it is worst for recently written vectors, which are the ones most likely to be written again. The lookup asks the frontend first and falls back to the backend, under the guards `relabelVector` takes and in the same order. Nothing about it is HNSW-specific -- both tiers are plain indexes -- so it lives on `VecSimTieredIndex`, where the SVS tiered index gets it too, and the HNSW-specific override is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

nonirosenfeldredis · 2026-08-26T11:33:40Z

-
-        auto id = labelToIdLookup.at(label);
+        auto it = labelToIdLookup.find(label);
+        if (it == labelToIdLookup.end()) {


fixed existing bug

codecov · 2026-08-26T11:37:04Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.38%. Comparing base (2546f8e) to head (aaeafd0).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1025      +/-   ##
==========================================
+ Coverage   97.30%   97.38%   +0.07%     
==========================================
  Files         141      141              
  Lines        8624     8653      +29     
==========================================
+ Hits         8392     8427      +35     
+ Misses        232      226       -6

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The accessor read `labelLookup` and the data blocks with no lock, which was tolerable while it was test-only and single-threaded, and is not now that production code can call it. A shared `mainIndexGuard` is not enough to make the read safe. Tiered ingest takes exactly that lock plus `indexDataGuard` and then stores a new element -- rehashing the map and possibly resizing the data blocks -- and `markDelete` mutates under the same inner guard. So a reader holding only the main lock can observe a rehash in progress or a block that has moved. The guard belongs to the accessor rather than its callers, matching `getLabelsSet`: the tiered lookup added in the previous commit holds the tier-selection guards and lets each tier's accessor take its own, which is the division `computeUnifiedIndexLabelsSetUnsafe` already relies on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Returning as soon as the flat buffer held the label was right only for a single-value index. In a multi-value one a label's vectors are routinely split across the tiers while an ingest job is pending, so the buffer alone is a subset and the already-ingested vectors were missing from the result. Which tiers to read now follows `getDistanceFrom_Unsafe`, which faced the same choice: short-circuit on a buffer hit only when the index is single-value, otherwise read the backend as well. Two properties a tiered read cannot avoid are documented rather than papered over. The vectors come out buffer-first, which for a split label is not insertion order; and an ingest job inserts into the backend before removing from the buffer, so a vector caught inside that window is reported by both tiers. `flatIndexGuard` is held across both reads -- it cannot prevent the duplicate, but it does stop the buffer's copy disappearing between them, which is what would turn a duplicate into an omission. The test covers the split, the ingested state, and an absent label. It fails on the previous implementation with the subset it returned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

cursor

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 d012e58. Configure here.}

Two ways the accessor could answer with values that are not there. A quantized element is smaller than its elements would be -- SQ8 keeps one byte per dimension plus a few metadata floats, against `dim * sizeof(DataType)` -- and vectors are packed back-to-back at `storedDataSize`. Copying the elements size therefore ran off the end of the element, into its neighbour or past the block for the last one, and reinterpreted the compression as floats. Nothing here dequantizes, so the size check now reports nothing instead, which the contract already defines as "cannot tell". The docstring claiming a reconstruction is corrected: there never was one. `SVSIndex` kept its `getDataByLabel` in a test-only block, asserting "Not implemented", and relied on the base's default outside it. A tiered SVS index reaches it through `VecSimTieredIndex::getDataByLabel` as soon as a vector is ingested, so that assert was reachable from a test build. It now has one unconditional definition that appends nothing, which is its honest answer -- SVS keeps vectors in the SVS library's reduced form and does not hand them back -- and with every concrete class defining the method the base goes back to pure virtual, so a future index type has to state its answer rather than inherit silence. Tests cover the quantized refusal and reading an absent label on both single-value implementations, where `.at()` used to throw. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Three clang-format violations, all in hunks this branch introduced: two brace/blank-line spots left behind where `hnsw_tiered.h` lost its own `getDataByLabel`, and an empty body in `svs.h` that ran to column 101. The SVS override was also the branch's only uncovered code -- nothing called it. It is worth a test of its own rather than a coverage exemption: the behaviour it pins is that SVS reports nothing rather than pretending, and the alternative to defining it here was what let a tiered SVS index reach a not-implemented stub. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…vector-data

dor-forer · 2026-08-27T07:16:12Z

    void getDataByLabel(
        labelType label,
        std::vector<std::vector<svs_details::vecsim_dt<DataType>>> &vectors_output) const override {
-        assert(false && "Not implemented");


just wondering and trying to make things clear for me - what is the expected behavior for a tiered multi-value SVS index when a label is split across the frontend and backend? SVSIndex::getDataByLabel() appends nothing, the tiered returns only the frontend vectors, and the caller cannot tell that the result is partial. Is a partial response acceptable for the intended caller? If not, should we disable this for SVS?

agreed, since both the svs methods: getRelabelData & relabelVector not implemented in SVS repo. lets return an error up front and not wait for the backend svs to respond.

I see that the current change skips the SVS backend but still returns vectors from the frontend, so it still returns the the partial result that looks successful. Should we ust block the SVS getDataByLabel at all, and allow until it is implemented?

I suppose for quantized backend that's also relevant to check the backend and returning right away instead of returning results only if the vector happened to be on the frontend. Disk HNSW might behave like that (if we return from in memory rather than go to disk for this API, it's probably not decided yet)

I gated it in the tiered class : checking for svs . when I added quantizied check - claude replied that i

ts not reachable to get here . Quantized: cannot be tested, and the check cannot fire. WHY? tiered_factory.cpp rejects a quantized backend outright — NewIndex returns nullptr and EstimateInitialSize throws "Quantization is not supported for tiered HNSW indexes", because the brute-force frontend isn't quantized and the stored layouts would be incompatible. I also checked the enterprise disk backend, which does store SQ8: it never sets isQuantized, so it reports false too. so no need for quantized

@dor-forer you can look now

The guard refusing to read a quantized element as values compared `getStoredDataSize()` against `dim * sizeof(DataType)`. That inequality only holds above a certain dimension: SQ8 stores one byte per dimension plus FP32 metadata, so at dim 4 with FP32/L2 the element is 20 bytes against 16 for the raw elements. Below the crossover the comparison concludes "not quantized" and hands back compression and metadata reinterpreted as floats, which is the outcome the guard exists to prevent. `VecSimIndexAbstract::isQuantized` is the reliable test, set by the factory alongside `storedDataSize` and already used for the same kind of branch in `HNSWIndex_Single::getDistanceFrom_Unsafe`. The test moves to dim 4, where the size comparison fails, and keeps a dim 40 case for the side it handled. The first version of this test used dim 40 only, which is why the bug survived it; the small-dimension case fails against the previous guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`getDataByLabel` took `flatIndexGuard` and `mainIndexGuard` with bare lock_shared / unlock_shared pairs, copying the style used elsewhere in the file. Both calls it wraps append to the caller's vector and so can throw `bad_alloc`, and on that path the manual unlock never runs -- leaving a reader guard held for the life of the process, which blocks every subsequent writer. Scoped guards also give the release order for free: main is released before flat, the reverse of the acquisition order the lock hierarchy requires. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`getDataByLabel` built each vector locally and then copied it into the caller's output -- a second allocation and memcpy per stored vector, on a path that is now production code rather than a test helper. The local is dead after the push, so it can be moved. `getStoredVectorDataByLabel` directly below each of these already moved; this brings the four `getDataByLabel` implementations in line with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Two things the tiered read got wrong about its own contract and its backend. It assumed `vectors_output` arrives empty, and the assumption was load-bearing: for a single-value index a non-empty output made the "did the buffer have it" test false, so the backend was skipped and the label's vector was missed entirely. The tier decision now compares against a size snapshot instead of testing emptiness, so it holds whatever the caller passes, and a BUILD_TESTS assertion catches the caller that passes a reused vector and would otherwise silently get two labels' vectors concatenated. It also read the backend for an SVS index, which reports nothing by construction. Reaching that read means waiting on `mainIndexGuard` -- behind an SVS batch update, potentially for a while -- to be told so. The read is now skipped for an SVS backend, and SVS's own accessor logs at debug level rather than staying silent. Both are temporary: the type test is deliberately explicit rather than dressed up as architecture, and the note to remove it sits on `SVSIndex::getDataByLabel`, where whoever implements it will be working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Both notes said what to delete without saying what to build. The one in svs.h now states what the method has to produce, why it is empty today, and the rule to carry over from the HNSW implementations: report nothing rather than a dequantized approximation, since a caller comparing stored bytes would read a reconstruction as a difference -- or as a match. It also records that implementing it does not by itself make an SVS-backed field relabel, because `relabelVector` is a separate gap. The tiered note lists what removal touches, and why the special case sits in the base class rather than as an override in `TieredSVSIndex`: the method is not virtual -- `VecSimIndexInterface` does not declare it -- and callers reach it through a `VecSimTieredIndex *`, so a derived override would never be found. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Reading the tiers only to have the backend append nothing costs `mainIndexGuard`, which on a tiered index can queue behind a long exclusive holder. Worse, reading the buffer alone is a *partial* answer for a multi-value label split across the tiers, and a caller cannot tell a subset from the whole -- reporting nothing is the contract's "cannot tell", half of it is a wrong answer. Two backends cannot report. A quantized one stores compression plus metadata and nothing here dequantizes; that check is permanent. SVS has no per-label read of its stored vectors yet, and that check goes away with MOD-17706 -- kept separate from the quantized one so removing it cannot take the permanent check with it. `isQuantized` is protected on `VecSimIndexAbstract`, and `VecSimTieredIndex` derives from `VecSimIndexInterface`, so it needed a public getter alongside the existing `isMultiValue()`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The tiered read had no test for either backend it skips. It has one now for SVS, with the vector deliberately left in the flat buffer -- the case where the frontend could have answered, which is what makes the test fail if the check or the no-fallback behaviour is undone. Checked against a build with the check disabled before trusting it. The quantized check goes away instead of gaining a test, because it cannot fire: `TieredFactory::NewIndex` returns nullptr for a quantized primary index and `EstimateInitialSize` throws, since the brute-force frontend is not quantized and the stored layouts would not match. A tiered index therefore never has a quantized backend. The guard that does the real work is the one inside the HNSW accessors, which covers the non-tiered path where quantization is reachable. `isIndexQuantized()` had no other caller and goes with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

RedisAI/VectorSimilarity#1025 is merged, so the submodule points at a commit on `main` (5bd11d1e) instead of that PR's branch head. Nothing else changes: the pinned tree carries the same relabelVector and getDataByLabel surface this branch was already building against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

nonirosenfeldredis and others added 2 commits August 26, 2026 14:18

cursor Bot reviewed Aug 26, 2026

View reviewed changes

Comment thread src/VecSim/vec_sim_tiered_index.h

Comment thread src/VecSim/vec_sim_tiered_index.h Outdated

nonirosenfeldredis commented Aug 26, 2026

View reviewed changes

nonirosenfeldredis and others added 2 commits August 26, 2026 14:44

nonirosenfeldredis requested review from alonre24 and ofiryanai August 26, 2026 11:56

cursor Bot reviewed Aug 26, 2026

View reviewed changes

Comment thread src/VecSim/algorithms/hnsw/hnsw_single.h

nonirosenfeldredis requested review from alonre24, dor-forer and ofiryanai and removed request for alonre24 and ofiryanai August 26, 2026 14:10

This was referenced Aug 26, 2026

[MOD-17688] Add a relabelVector API #1026

Closed

[MOD-17688] Relabel an unchanged vector instead of re-adding it on update RediSearch/RediSearch#11195

Open

nonirosenfeldredis enabled auto-merge August 27, 2026 06:14

Merge remote-tracking branch 'origin/main' into sharon-MOD-17688-get-…

0906311

…vector-data

dor-forer reviewed Aug 27, 2026

View reviewed changes

nonirosenfeldredis and others added 4 commits August 27, 2026 13:22

nonirosenfeldredis requested a review from dor-forer August 27, 2026 12:27

nonirosenfeldredis and others added 3 commits August 27, 2026 15:49

ofiryanai approved these changes Aug 27, 2026

View reviewed changes

nonirosenfeldredis added this pull request to the merge queue Aug 27, 2026

Merged via the queue into main with commit 5bd11d1 Aug 27, 2026
23 of 25 checks passed

nonirosenfeldredis deleted the sharon-MOD-17688-get-vector-data branch August 27, 2026 17:02

Navigation Menu

Uh oh!

Let production code read a label's stored vectors - #1025

Let production code read a label's stored vectors#1025
nonirosenfeldredis merged 14 commits into
mainfrom
sharon-MOD-17688-get-vector-data

nonirosenfeldredis commented Aug 26, 2026 •

edited by cursor Bot

Loading

Uh oh!

Uh oh!

Uh oh!

nonirosenfeldredis Aug 26, 2026

Uh oh!

codecov Bot commented Aug 26, 2026 •

edited

Loading

Uh oh!

cursor Bot left a comment

Uh oh!

Uh oh!

Uh oh!

dor-forer Aug 27, 2026

Uh oh!

nonirosenfeldredis Aug 27, 2026

Uh oh!

dor-forer Aug 27, 2026 •

edited

Loading

Uh oh!

ofiryanai Aug 27, 2026 •

edited

Loading

Uh oh!

nonirosenfeldredis Aug 27, 2026 •

edited

Loading

Uh oh!

nonirosenfeldredis Aug 27, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

nonirosenfeldredis commented Aug 26, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nonirosenfeldredis Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dor-forer Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

nonirosenfeldredis Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

dor-forer Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ofiryanai Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nonirosenfeldredis Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nonirosenfeldredis Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nonirosenfeldredis commented Aug 26, 2026 •

edited by cursor Bot

Loading

codecov Bot commented Aug 26, 2026 •

edited

Loading

dor-forer Aug 27, 2026 •

edited

Loading

ofiryanai Aug 27, 2026 •

edited

Loading

nonirosenfeldredis Aug 27, 2026 •

edited

Loading