Skip to content

fix(vector): read on-disk embeddings through the scan state's transaction - #101

Merged
adsharma merged 1 commit into
LadybugDB:mainfrom
aikins01:fix/vector-scan-state-transaction
Oct 9, 2026
Merged

adsharma merged 1 commit into
LadybugDB:mainfrom
aikins01:fix/vector-scan-state-transaction

Conversation

@aikins01

@aikins01 aikins01 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

OnDiskEmbeddings::constructScanState takes an optional transaction that overrides the one the embeddings were built with, but getEmbedding and getEmbeddings ignored it and used the member transaction for visibility, the committed/uncommitted source, scan-state setup and the lookup. A caller passing a different transaction would get visibility from one transaction and rows from another. Every current caller passes the same transaction, so nothing changes today.

OnDiskEmbeddingScanState now keeps the transaction it was built with, and both lookup paths read through it, the same way the cached quantized embeddings already resolve visibility.

Depends on #95 and should merge after it; the first commit here is #95's. Like #95, CI builds against the latest successful core Build and Deploy run on main, which predates LadybugDB/ladybug#1113, so it won't compile until that workflow next succeeds.

@adsharma

adsharma commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Follow-ups (non-blocking, reviewed pr95..pr101):

  1. Adopt Transaction::tryGetLocalRowIdx (core perf(transaction): resolve a node's local row with one local-table lookup ladybug#1155, merged) in getEmbedding — replaces the isUnCommitted + getLocalRowIdx pair (2–3 getLocalTable lookups) with one. That core PR explicitly names this call site.
  2. Consider DASSERT(transaction != nullptr) in OnDiskEmbeddingScanState ctor — TableBackedQuantizedEmbeddings::constructScanState has one; here a null member + null override would null-deref at isUnCommitted instead of failing fast.
  3. Pre-existing asymmetry, out of scope: single path uses isVisibleNoLock, batch path uses locking isVisible, and batch never flips source/nodeGroupIdx itself (relies on lookupMultiple internals). Flagging so it isn't lost.

…tion

OnDiskEmbeddings::constructScanState accepts a transaction that overrides
the one the embeddings were built with, but getEmbedding and
getEmbeddings ignored it: visibility, the committed/uncommitted source,
scan-state setup, and the lookup all used the member transaction. A
caller passing a different transaction would read visibility from one
transaction and rows from another.

OnDiskEmbeddingScanState now keeps the transaction it was constructed
with, and both lookup paths use it, matching how the cached quantized
embeddings already resolve visibility.
@adsharma
adsharma force-pushed the fix/vector-scan-state-transaction branch from 268136a to 1ed269e Compare October 9, 2026 18:22
@adsharma
adsharma merged commit 4a4c15e into LadybugDB:main Oct 9, 2026
2 checks passed
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