Skip to content

Allow storeNewElement to reuse an existing element id - #1010

Draft
nonirosenfeldredis wants to merge 1 commit into
mainfrom
sharon-9645-reuse-id
Draft

Allow storeNewElement to reuse an existing element id#1010
nonirosenfeldredis wants to merge 1 commit into
mainfrom
sharon-9645-reuse-id

Conversation

@nonirosenfeldredis

Copy link
Copy Markdown
Collaborator

Describe the changes in the pull request

HNSWIndex::storeNewElement always allocated a fresh id by advancing curElementCount, and
appended 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 elementId parameter:

  • INVALID_ID (the default) - unchanged behaviour: a fresh id is allocated via
    curElementCount++, the index grows by a block first if it is full, and the vector and the
    graph data are appended (RawDataContainer::addElement / DataBlock::addElement).
  • A valid id - the id is used as-is, curElementCount is left untouched, no capacity check or
    growth happens, and the vector and the graph data are written in place over that slot
    (RawDataContainer::updateElement / DataBlock::updateElement). The in-place path is required
    because both containers only support appending at curElementCount -
    DataBlocksContainer::addElement asserts id == element_count.

Everything downstream of the id already works by id and needed no change: idToMetaData[id] is
reset to ElementMetaData(label) (which re-raises IN_PROCESS), setVectorId re-publishes the
label mapping, and the entry-point / maxLevel promotion 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's
ElementGraphData must have already been destroyed, otherwise its link lists are leaked by the
overwrite. Note also that the slot's elementLocks[elementId] is not taken here, so the caller
must guarantee the element is unreachable by concurrent scans - storeNewElement's usual contract
(indexDataGuard held 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 of TieredHNSWIndex::insertVectorToHNSW.

Which issues this PR fixes

  1. MOD-9645

Main objects this PR modified

  1. HNSWIndex::storeNewElement (src/VecSim/algorithms/hnsw/hnsw.h) - optional elementId
    parameter 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

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

Internal (non-public) API only: storeNewElement gains a defaulted parameter, so every existing
caller and the public VecSim API are unaffected.

`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>
@CLAassistant

CLAassistant commented Aug 9, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

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.

2 participants