Let production code read a label's stored vectors - #1025
Open
nonirosenfeldredis wants to merge 4 commits into
Open
Let production code read a label's stored vectors#1025nonirosenfeldredis wants to merge 4 commits into
nonirosenfeldredis wants to merge 4 commits into
Conversation
`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>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| auto id = labelToIdLookup.at(label); | ||
| auto it = labelToIdLookup.find(label); | ||
| if (it == labelToIdLookup.end()) { |
Collaborator
Author
There was a problem hiding this comment.
fixed existing bug
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1025 +/- ##
==========================================
+ Coverage 97.18% 97.31% +0.12%
==========================================
Files 141 141
Lines 8537 8644 +107
==========================================
+ Hits 8297 8412 +115
+ Misses 240 232 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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>
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 d012e58. Configure here.
| // Only copy the vector data (dim * sizeof(DataType)), not any additional metadata like the | ||
| // norm | ||
| memcpy(vec.data(), this->getDataByInternalId(id), this->dim * sizeof(DataType)); | ||
| memcpy(vec.data(), this->getDataByInternalId(it->second), this->dim * sizeof(DataType)); |
There was a problem hiding this comment.
Quantized label read overruns storage
High Severity
Promoting getDataByLabel to production still copies dim * sizeof(DataType) from internal storage. On SQ8 HNSW, stored blobs are about one byte per dimension plus metadata, so this over-reads the element and invents float values. The new docs claim quantized reconstruction, but nothing dequantizes; equality checks and any standalone SQ8 caller can crash or always treat vectors as changed.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d012e58. Configure here.
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
1 participant
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
Makes
getDataByLabelusable outsideBUILD_TESTS, so production code can ask what alabel 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_Unsafereturns 0 for distinct vectors under IP, and undercosine 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:
labelToIdLookup.at(), which throws; the multi-value ones dereferencedfind()withoutcomparing to
end(), which is undefined behaviour. Both now leave the output empty, sothe output size answers whether the label is held.
TieredHNSWIndex::getDataByLabelconsulted the backend alone, so everything written sincethe 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
relabelVectortakes and in the same order. Nothing about it is HNSW-specific, so it movedto
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
Main objects this PR modified
VecSimIndexAbstract::getDataByLabel— out ofBUILD_TESTS, defaulted rather than pureBruteForceIndex_Single/_Multi,HNSWIndex_Single/_Multi— same, and absence-safeVecSimTieredIndex::getDataByLabel— new, asks the tier holding the labelTieredHNSWIndex::getDataByLabel— removed, the base covers itMark if applicable
Testing
cteston this branch: 2759/2759 passed, including the 97 tiered HNSW tests. No test calledthe tiered wrapper's
getDataByLabelbefore this change — the seven existing uses all call atier 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
Production callers now depend on tiered two-tier merge semantics and documented duplicate/race windows; HNSW locking changes affect concurrent read paths during background ingest.
Overview
getDataByLabelis now a production API onVecSimIndexAbstract(default no-op) instead of a test-only pure virtual, so callers can compare stored vector bytes on update rather than relying on distance.getStoredVectorDataByLabelstays behindBUILD_TESTS.Absent labels are safe: brute-force and HNSW single/multi implementations use
findand return without appending, so empty output means the label is missing. HNSW reads take a sharedindexDataGuardbecause ingest/delete can mutate label maps under only a shared main lock.Tiered indexes implement lookup in
VecSimTieredIndex: read the flat buffer first, then the backend when multi-value or the buffer had nothing—fixing “missing” labels still waiting in the flat tier. The HNSW tiered wrapper’s duplicate method is removed. A unit test covers multi-value labels split across tiers during async ingest.Reviewed by Cursor Bugbot for commit d012e58. Bugbot is set up for automated code reviews on this repo. Configure here.