Skip to content

fix: reject missing merge insert update addresses - #8168

Open
lance-gatekeeper[bot] wants to merge 2 commits into
mainfrom
gatekeeper/fix-7934-1
Open

fix: reject missing merge insert update addresses#8168
lance-gatekeeper[bot] wants to merge 2 commits into
mainfrom
gatekeeper/fix-7934-1

Conversation

@lance-gatekeeper

@lance-gatekeeper lance-gatekeeper Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • reject merge insert update addresses that are missing from the target fragment instead of silently keeping the original row
  • require deletion-free, exact live-address coverage before using the full-fragment direct rewrite
  • cover missing addresses between live rows, after the final live row, and in the full-fragment shortcut

Root cause

Partial-fragment reconciliation used a debug-only assertion when the next update address had already been passed. Release builds replaced that update with the original row, and addresses after the final live row were never checked. The equal-row-count full-fragment shortcut also bypassed reconciliation without validating addresses, so a stale scalar-index result could silently write a nonexistent row payload into another physical position.

Validation

  • cargo test -p lance test_updated_row_addr_missing
  • cargo fmt --all -- --check
  • cargo clippy --all --tests --benches -- -D warnings

Fixes #7934

@github-actions github-actions Bot added the bug Something isn't working label Aug 3, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: request changes. The fail-closed address invariant must cover both partial and full-fragment update paths. Validate exact live-address coverage before selecting the full-rewrite optimization, or route ambiguous cases through reconciliation, so every writer path rejects unresolved updates.

}
let mut updated_row_addr_iter = get_row_addr_iter(&batches).peekable();
let mut updated_rows =
UpdatedRowAddrReconciler::new(get_row_addr_iter(&batches));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Address validation is constructed only after the full-fragment rewrite branch, so equal row counts still bypass it. On a two-row fragment, updates for (frag, 0) and nonexistent (frag, 2) take that shortcut and return Ok; the second payload is written as physical row 1 instead of rejecting the missing address. Require deletion-free, exact contiguous address coverage before the shortcut, or reconcile this branch too.

Reproducer

On 67d2f61072ba11cd323fa876ebcbc730d5d190ee, I added a disposable test that calls update_fragments with those two addresses and asserts result.is_err(), then ran:

CARGO_TARGET_DIR=/home/agent/tmp/implementation-8168-target cargo test -p lance dataset::write::merge_insert::tests::test_disposable_full_fragment_rewrite_rejects_missing_row_address -- --exact --nocapture

It exited 101 with full-fragment rewrite accepted a missing row address (0 passed; 1 failed).

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: approve. The revision closes the full-fragment bypass by requiring deletion-free, exact contiguous address coverage before direct rewrite; invalid sets fall back to reconciliation. This preserves the optimization without weakening the fail-closed update contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

merge_insert can silently skip updates when a stale scalar index returns a missing row address

0 participants