Skip to content

Add an opaque userData slot to VecSimDiskContext - #1013

Merged
GuyAv46 merged 2 commits into
RedisAI:mainfrom
kei-nan:jk-disk-context-user-data
Aug 16, 2026
Merged

Add an opaque userData slot to VecSimDiskContext#1013
GuyAv46 merged 2 commits into
RedisAI:mainfrom
kei-nan:jk-disk-context-user-data

Conversation

@kei-nan

@kei-nan kei-nan commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

VecSimDiskContext is a pass-through: the embedder fills it, vecsim forwards it to the disk index, and the storage layer behind storage is what interprets the contents.

Storage implementations that keep several indexes in one physical store need to know which of them a handle belongs to. The index name is already available, but it is variable-width, which makes it a poor fit for a key prefix.

This adds uint32_t userData for that purpose — forwarded unchanged and never interpreted here.

Two deliberate choices:

  • A value, not a pointer. storage is a void * because it is genuinely a handle to a live object; this is a small identifier, so a pointer would import lifetime and ownership questions across the boundary for no benefit.
  • 32 bits, sorted into the struct by width. Placed above rerank so fields run widest-first, which leaves VecSimDiskContext at 32 bytes — the same size as before this PR, so the slot costs nothing. The only value it carries today is a 16-bit field index; stopping at u32 keeps headroom without pinning the embedder's current width into a public struct, and narrowing to u16 would shrink nothing, since the trailing bool pads both widths out the same.

Reordering is safe: every caller builds this struct with designated initializers, and none sets userData yet. Checked with static asserts that sizeof(VecSimDiskContext) == 32, offsetof(userData) == 24, and that a C++20 designated initializer in the new order still compiles.

The meaning of the value is documented on the consumer side, where it is interpreted, rather than here.

Self-contained: no other change is required for this to be correct, and nothing in this repository reads the field.


Note

Low Risk
Additive public struct field with no in-repo consumers or behavior changes; callers must zero-initialize new fields when constructing the struct.

Overview
Extends the public VecSimDiskContext struct with a uint32_t userData field so embedders can pass a fixed-width index identifier into the disk storage layer (e.g. when several logical indexes share one physical store), without relying on variable-length indexName for keying.

VecSim does not interpret userData; it is only documented as forwarded unchanged with the rest of the disk context. rerank is unchanged aside from comment formatting.

Reviewed by Cursor Bugbot for commit 567031e. Bugbot is set up for automated code reviews on this repo. Configure here.

VecSimDiskContext is a pass-through: the embedder fills it, vecsim forwards it
to the disk index, and the storage layer behind `storage` is what interprets the
contents. Storage implementations that keep several indexes in one physical
store need to know which of them a handle belongs to, and the index name alone
is variable-width, so it is a poor fit for a key prefix.

`userData` gives the embedder a fixed-width slot for that, forwarded unchanged
and never interpreted here. It is a value rather than a pointer so it carries no
lifetime or ownership across the boundary, and 64 bits leaves headroom so a
later need does not have to change a public struct again.
@CLAassistant

CLAassistant commented Aug 16, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Comment thread src/VecSim/vec_sim_common.h Outdated
Addresses review on VecSimDiskContext:

- Trim the two-line comment down to a one-liner that names who writes
  the field and who consumes it.
- uint64_t -> uint32_t. The only value the slot carries today is a
  16-bit field index; u32 keeps headroom without costing a byte, since
  the trailing bool pads either width out to the same size.
- Move it above rerank so fields run widest-first. With that ordering
  the struct stays 32 bytes, so the slot is free.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kei-nan
kei-nan requested a review from GuyAv46 August 16, 2026 09:26
@GuyAv46
GuyAv46 merged commit 4d5644a into RedisAI:main Aug 16, 2026
11 of 14 checks passed
kei-nan added a commit to RediSearch/RediSearch that referenced this pull request Aug 16, 2026
RedisAI/VectorSimilarity#1013 has merged, so the gitlink no longer has to
reference a fork branch that CI cannot fetch. Pin the squash commit on
main instead; the header content is identical to the fork commit it
replaces.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pull Bot pushed a commit to Stars1233/RediSearch that referenced this pull request Aug 16, 2026
…layer (RediSearch#10933)

* [MOD-17606] Pass the vector field's schema index to the disk storage layer

The disk storage layer can keep several of an index's vector fields in one
physical store, in which case it needs a fixed-width identifier per field to
keep their keys apart. The field name it already receives is variable-width, so
it is a poor fit for that.

`VecSimDiskContext` gains an opaque `userData` slot for exactly this kind of
pass-through (see the VectorSimilarity bump), and both places that build a
field's disk context fill it with `fs->index`. That is the natural choice: the
field index is unique per field regardless of type, sequential from 0, and
restored in the same order on RDB load, so it is stable across a restart —
which is also why the disk layer already keys its TAG and NUMERIC storage by
it.

* Track the reviewed VecSimDiskContext layout

Review on VectorSimilarity#1013 narrowed userData to uint32_t and moved
it ahead of rerank so the struct's fields run widest-first, which keeps
VecSimDiskContext at 32 bytes. Bump deps/VectorSimilarity to that commit
and set userData before rerank here to match: C is indifferent to
designated-initializer order, but C++20 is not, and keeping the two in
step avoids a trap for the next caller.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Repoint deps/VectorSimilarity at upstream main

RedisAI/VectorSimilarity#1013 has merged, so the gitlink no longer has to
reference a fork branch that CI cannot fetch. Pin the squash commit on
main instead; the header content is identical to the fork commit it
replaces.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

3 participants