Allow storeNewElement to reuse an existing element id - #1010
Draft
nonirosenfeldredis wants to merge 1 commit into
Draft
Allow storeNewElement to reuse an existing element id#1010nonirosenfeldredis wants to merge 1 commit into
nonirosenfeldredis wants to merge 1 commit into
Conversation
`storeNewElement` always allocated a fresh id by advancing `curElementCount` and appended the vector and the graph data to their containers. Add an optional `elementId` parameter: when a valid id is given it is used as-is, the index does not grow, and both the vector and the graph data are written in place over that slot (`RawDataContainer::updateElement` / `DataBlock::updateElement`), since the containers only support appending at `curElementCount`. Defaults to INVALID_ID, so all existing call sites keep allocating fresh ids. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
HNSWIndex::storeNewElementalways allocated a fresh id by advancingcurElementCount, andappended the vector and the graph data to their containers. There was no way to store an element
into an id that the caller already holds - e.g. an already-freed slot one wants to reuse instead
of growing the index.
This PR adds an optional
elementIdparameter:INVALID_ID(the default) - unchanged behaviour: a fresh id is allocated viacurElementCount++, the index grows by a block first if it is full, and the vector and thegraph data are appended (
RawDataContainer::addElement/DataBlock::addElement).curElementCountis left untouched, no capacity check orgrowth happens, and the vector and the graph data are written in place over that slot
(
RawDataContainer::updateElement/DataBlock::updateElement). The in-place path is requiredbecause both containers only support appending at
curElementCount-DataBlocksContainer::addElementassertsid == element_count.Everything downstream of the id already works by id and needed no change:
idToMetaData[id]isreset to
ElementMetaData(label)(which re-raisesIN_PROCESS),setVectorIdre-publishes thelabel mapping, and the entry-point /
maxLevelpromotion is untouched.Caller contract for the reuse path (documented at the declaration): the slot must be a legal,
already-freed one -
elementId < curElementCount(asserted) and the previous occupant'sElementGraphDatamust have already been destroyed, otherwise its link lists are leaked by theoverwrite. Note also that the slot's
elementLocks[elementId]is not taken here, so the callermust guarantee the element is unreachable by concurrent scans -
storeNewElement's usual contract(
indexDataGuardheld exclusive) is not sufficient on its own for the reuse path.All three existing call sites are unchanged and keep allocating fresh ids:
HNSWIndex::storeVector, and the two branches ofTieredHNSWIndex::insertVectorToHNSW.Which issues this PR fixes
Main objects this PR modified
HNSWIndex::storeNewElement(src/VecSim/algorithms/hnsw/hnsw.h) - optionalelementIdparameter plus the in-place store path
Testing
Full unit suite green locally (macOS arm64, RelWithDebInfo):
100% tests passed out of 2556.No new test yet - the reuse path currently has no caller, so coverage should land together with
the first one.
Mark if applicable