Skip to content

fix(vector): use owner-scoped transaction overloads for embedding scans - #95

Open
aikins01 wants to merge 1 commit into
LadybugDB:mainfrom
aikins01:adapt-owner-scoped-transaction-api
Open

aikins01 wants to merge 1 commit into
LadybugDB:mainfrom
aikins01:adapt-owner-scoped-transaction-api

Conversation

@aikins01

@aikins01 aikins01 commented Oct 4, 2026

Copy link
Copy Markdown

Summary

OnDiskEmbeddings::getEmbedding checked whether an offset falls in a node table's uncommitted range via the table-ID-keyed Transaction::isUnCommitted / getLocalRowIdx overloads. Core PR LadybugDB/ladybug#1113 removes those table-ID-keyed lookups in favor of table-scoped ones, because table IDs are only unique per catalog, so a table-ID-only lookup is ambiguous when a transaction touches several attached graphs.

This PR switches the two call sites to the table-scoped overloads (nodeTable is already in scope at both).

Dependency

Builds on top of LadybugDB/ladybug#1113 — the core branch also restores the old table-ID overloads as a compatibility shim, so this change can land after it at any time.

Test plan

  • minimal linux extension test on the core PR compiles the pinned submodule with the compatibility shim
  • Change is mechanical: same two expressions, now resolving the owning table directly

@adsharma

adsharma commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Looks good. Two minor nits:

  1. Redundant lookups on a hot path (pre-existing, not introduced): isUnCommitted(table) + getLocalRowIdx(table) →
    getMinUncommittedNodeOffset(table) does ~3-4 unordered_map lookups per getEmbedding. Consider a single
    Transaction::tryGetLocalRowIdx(const Table&, offset) -> optional<row_idx> helper in core. Leave for follow-up.
  2. Stale-transaction smell (pre-existing, out of scope): constructScanState(transaction_) accepts an override, but getEmbedding uses member transaction for isVisibleNoLock/isUnCommitted/lookup. If a caller ever passes a different txn, visibility and source disagree. Worth a DASSERT or threading scanState.transaction through — separate PR.

@aikins01

aikins01 commented Oct 7, 2026

Copy link
Copy Markdown
Author

Thanks! Agreed both are pre-existing and out of scope here.

A single Transaction::tryGetLocalRowIdx(const Table&) helper in core sounds right, and the constructScanState/getEmbedding mismatch deserves its own change since the fix touches the scan-state contract. I'll take both as follow-ups, the helper once LadybugDB/ladybug#1113 lands.

This PR stays blocked on that core PR meanwhile; the failing build here runs against core main, which doesn't carry the owner-scoped overloads yet.

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