Repository navigation
perf(engine): delete a branch merge's rows by the _rowid its walk read - #1301
Conversation
A branch merge staged each delete chunk as `id IN (...)`, so Lance searched the `id` BTREE and scanned the `id` column of every fragment the index did not cover. The rows a merge deletes were already read with their `_rowid` from the target (three-way and lineage) or from the base the target still pins (adopt). Each delete chunk now carries those row ids and stages `_rowid IN (...)`, which Lance's scanner turns into a take: one row read per deleted row, no index and no data column. The ids stay for the change record. Closes ModernRelay#1295
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: ready. No blocking findings. Three optional suggestions follow.
This review covers head 4396d615d7f89a77c65ac82028a9ef91e5302d1b (base 2d8effabe1900f63c30ecbf918c8fcfe3080f025).
Why, what, how, impact
Why. A branch merge deleted each chunk of rows with id IN (...). Lance runs that filter as a scan. It searches the id BTREE and then reads the id column of every fragment the index does not cover, once per chunk. So the cost of a merge delete grew with the unindexed tail of the table, not with the number of deleted rows.
What. Each delete chunk now keeps the _rowid that the target-side read returned for each row. The delete stages _rowid IN (...). Lance turns that into a take by row id. The ids still go to the change record.
How it stays correct. The change replaces a logical selector (id) with a physical one (_rowid). That is safe only if a _rowid read at the target pin still names the same live row when the delete link runs, after the insert and update links of the same chain. I checked this against the pinned Lance 13.0.0 source:
- A strict insert appends new fragments. It does not touch existing rows.
KnownPresentUpdaterunsmerge_insertwithUpdateAlland full rows, so it takes theRewriteRowspath (lance-13.0.0/src/dataset/write/merge_insert.rs, theelsearm near lines 2958-3060). It writes the updated rows to new fragments and adds deletion vectors to the old ones (apply_deletions). A fragment is removed only when every one of its rows is deleted, and a fragment that still holds a row this merge will delete has a live row. The partial-schemaRewriteColumnspath (the column patch that #1272 introduces for mutations) patches columns in place and does not move rows either.- Graph tables use stable row ids (
docs/dev/lance.md, "Stable row IDs"). An update keeps the row's_rowid, and the row-id index of the new version maps it to the live address. The classifier never updates a row it deletes. - Lance's delete already resolved its scanned
_rowids through the same row-id index before this change (lance-13.0.0/src/dataset/write/delete.rs:328). The new form removes the column scan and adds no new cost class. - The adopt path takes the
_rowidfrom the base row. That is valid because adopt-with-delta runs only whensame_manifest_state(base, target)holds (same path, published version and native branch), and it publishes onto the verified target handle. - Nothing reads the Delete transaction's predicate text back.
changes/candidate_scan.rs:253ignores it and reads the recorded deleted ids.
Impact. One delete chunk costs one take per deleted row, whatever the index coverage. The surface guard pins this. No public API, file format or documented behavior changes.
Blocking
None.
Optional
-
Make a partial match fail closed.
commit_staged_delete_chunks(merge.rs:4557) refuses a chunk that matches no rows. It then records every id of the chunk in the change record (merge.rs:4577), whatever Lance actually deleted. The PR description names this gap: a partly matching chunk is not caught. Withid IN, a partial match needed a logical id that the classifier had just read to be absent. With_rowid IN, it needs the physical mapping to drift. The argument above shows that cannot happen on Lance 13, but nothing checks it at run time. If it did happen, the merge would keep a row and publish a change record that says the row was deleted. That is the silent partial result that invariant 8 rejects.DeleteBuilder::execute_uncommittedalready returnsnum_deleted_rows, andstage_deletesees it (table_store.rs:3343). Compare it withchunk.row_ids.len()and fail with a typed internal error when they differ. Cost: one integer comparison and a small change to whatstage_deletereturns. Gain: the assumption this PR adds becomes a checked invariant, and a future Lance bump that changes update layout fails loudly instead of publishing a wrong change record. -
Make the two chunk kinds two types.
DeleteIdChunksnow holds two kinds of chunk: lineage candidate chunks (ids only) and delete chunks (ids and_rowids). An emptyrow_idsvector tells them apart at run time (merge.rs:145-160). Three run-time checks guard the mix: the mixed-chunk refusal (merge.rs:298), the length check indelete_expr(merge.rs:348), and the refusal of a candidate chunk as a delete. Two of them have their own unit assertions. If delete chunks hold(id, row_id)pairs in their own type, and candidate chunks keep ids only, these states cannot be built, and the three checks and their assertions go away. The shared parts (row bound, retained-byte bound, chunk boundary as transaction boundary) can stay in one helper. Also, a delete chunk is still bounded by the byte length of theid IN (...)rendering, but that string no longer goes to Lance for a delete. It now bounds the change record and keeps the pre-minted transaction plan unchanged. That is a valid reason, but the doc comment says the bound exists because it is "the form the lineage candidate path filters by", which does not explain why a delete chunk keeps it. After five more changes like this one, a reader will not know which bound protects which consumer. One sentence per bound would fix that. -
Test the case the description calls risky. The description says the input that would expose a mistake is a merge whose
KnownPresentUpdatelink rewrites a row in the same fragment as a row the same merge deletes.merge_delete_selects_rows_by_rowid_issue_1295deletes two rows and inserts on the target, so the update link does not run. I added this case temporarily to the same test module (not pushed). The feature branch deletesdv-0anddv-63and updatesdv-1anddv-62(all in one fragment). The target updatesdv-2and inserts a row. The test checks row counts and values formerge_lineage=off,onandverify. It passes on this head (and, as expected, on the base, which deletes by id). That shows the argument above holds in practice. Two more mutations in the existing test would keep that proof in the suite:set_ageon a source row next to the deleted ones, and one on the target. It would also be useful to addverifyto the lineage loop, because verify mode now compares(id, _rowid)pairs.
A note, not a request for this PR: the description says an object-store IOTracker around the dataset did not see the scanner's data reads. If that holds in general, the IoCounts curves and the DST storage-cost golden can understate scan costs everywhere, not only here. That deserves its own issue.
Alignment with other open work
- #1272 adds a column patch for mutation updates. It keeps row addresses, so it does not break the
_rowidargument, even if a merge later uses it. - #1299 and #1300 conflict only in the regenerated
docs/releases/v0.15.0.md, as the description says. - A future Lance that changes
TakeOperation::try_from_expr(for example, the IN-list shape it accepts afterExprSimplifier) would bring back the scan, not a wrong result.delete_chunk_reads_by_filter_form_issue_1295catches that at the bump.
Liability
The change removes a cost that grew with the unindexed tail, and it adds no API, state on disk, or format. It adds one physical assumption: a target-side _rowid names the same live row through the end of the merge chain. Today the assumption is proved by reading the code, not checked at run time. Suggestion 1 makes it checked. Suggestion 2 removes the run-time mode flag this PR adds to DeleteIdChunks.
Validation
Local runs on a Linux gVisor host, debug build, RUST_MIN_STACK=16777216 as in CI, through cargo dev-nextest in a clean worktree:
| Commit | Tests | Result |
|---|---|---|
4396d615 (head) |
merge_cost::merge_delete_selects_rows_by_rowid_issue_1295, lance_surface_guards::delete_chunk_reads_by_filter_form_issue_1295, chain_limit_tests::branch_merge_delete_ids_split_on_row_and_escaped_byte_bounds, chain_limit_tests::branch_merge_delete_chunks_chain_one_link_each, plus the temporary same-fragment-update test |
5 passed |
base 2d8effab engine source (crates/omnigraph/src) with the head's tests |
merge_delete_selects_rows_by_rowid_issue_1295 |
failed for the predicted reason: Lance recorded the filter __id IN ([Utf8("dv-0"), Utf8("dv-63")]) |
| same | surface guard and the temporary test | passed (the guard measures Lance itself; the temporary test checks rows, which both forms delete correctly) |
All temporary source and test edits are restored. CI on this head is green, including Test Workspace, GQ Logic Tests, DST pinned suite and both RustFS S3 jobs. I did not rerun the full merge suite, the GQT corpus or the DST suite locally; those results are CI's. I did not measure the cost claims myself. They rest on the checked-in surface guard (Lance analyze_plan) and on reading the Lance source.
GQT is not the right owner for this regression. The defect is the filter form Lance runs and the I/O it costs, not a row, count or error that a .gqt case can see. Both readings return the same rows. The _issue_1295 Rust test that inspects the Delete transaction is the cheapest owner that fails before the fix.
…t holds stage_delete now keeps the row count Lance deleted, and each branch-merge delete chunk must remove exactly its rows before its change record is written. Delete chunks (id and _rowid) and lineage candidate chunks (ids only) become two types of one IdChunks<R>, so a candidate chunk cannot build a delete and the mixed-chunk checks go away. The doc comment gives one reason per bound.
|
Thanks for the review of
Your note on
Local checks
Base main |
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: ready. No blocking findings. Two small optional notes follow.
This is a delta review. It covers head a6334ad945441e6f471a431e88de9b21b1496355. My previous review covered 4396d615. The delta is f62d3b94 (items 1 and 2) and a6334ad9 (item 3), +207/-116 in five files.
Why, what, how, impact
Why. The previous head deleted merge rows by _rowid. That was correct by argument, but nothing checked it at run time. A delete chunk also shared one type with lineage candidate chunks, and run-time checks kept the two kinds apart.
What.
stage_deletenow keeps Lance'snum_deleted_rowson the staged write.commit_staged_delete_chunksrefuses a chunk when that count differs from the chunk's row count. This happens before the chunk's change record is written or its detached transaction is committed.IdChunks<R>replaces the mixed type.DeleteIdChunks = IdChunks<u64>carries one_rowidper id.CandidateIdChunks = IdChunks<()>carries ids only.delete_exprexists only on the delete type. The mixed-chunk refusal, the length check and their two unit assertions are gone. The doc comment now gives one reason per bound.merge_delete_selects_rows_by_rowid_issue_1295now runsoff,onandverify. The source updates rows in the same fragment as the rows it deletes, and the target updates one too.
How the new check covers the failure class. I checked this against the pinned Lance 13.0.0 source:
DeleteJobbuilds a scan with the chunk's filter._rowid IN (...)becomesTakeOperation::RowIds.take_sourceturns the ids into an allow-mask (row_ids_as_take_input->mask_as_take_input,lance-13.0.0/src/dataset/scanner.rs:3839-3865). A_rowidwith no live row is not read. It is not an error.num_deleted_rowsisremoved_row_addrs.len(), the rows the scan actually returned (lance-13.0.0/src/dataset/write/delete.rs:310-333). So a stale_rowidlowers the count, and the new check fails the merge.- Within one table lineage, a stale
_rowidcannot land on a different live row. Lance assigns stable row ids from the monotonicnext_row_idcounter and does not reuse them (row ID lineage). So, for reads of the target's own pin, the only possible drift is a missing row, and the count check catches it. The cross-lineage case (two branches that allocate the same id) is still excluded by design: only a target-side read may supply a_rowid, as theDeleteIdChunks::pushcomment says. A count check cannot see that case, so it depends on that design rule.
Impact. A future Lance change that moves rows under a _rowid now fails the merge with a typed internal error. Before, it published a change record for a delete that did not happen (invariant 8). The cost is one integer comparison per chunk. No API, on-disk state or format changes. The type split removes a run-time mode flag and three checks, and adds no new state.
Blocking
None.
Optional
-
The comment claims more than the check proves. The comment at merge.rs:4577-4580 says "Lance must have removed exactly the chunk's rows". The code compares counts, not row identity. That is enough here only because of the monotonic-row-id argument above. One clause would keep the next reader from relying on the check for more than it does, for example: "a count match is enough because stable row ids are never reused within a lineage, and only a target-side read supplies them."
-
deleted_rowshas two sources. Instage_deleteit is Lance's count of the rows its scan returned. Instage_delete_rows(table_store.rs:3417) it is the number of addresses the caller passed. Both are correct today: the only reader is the merge, which usesstage_delete. The mutation path (exec/staging.rs:1004) takes its addresses from its own scan at the same version. But the field doc (table_store.rs:1055) says "as Lance counted them" for both. If a later caller trusts it afterstage_delete_rows, it gets the requested count, not a checked one. Either say so on the field, or set it only instage_delete.
A note for whoever merges second: this PR and #1272 and #1235 each append a guard at the end of crates/omnigraph/tests/lance_surface_guards.rs, so git merge-tree shows a text conflict there. Keep both sides. This was already true at 4396d615. My previous review missed it.
Alignment with other open work
Same as in my previous review. #1272 (column-patch updates) keeps row addresses, so the _rowid argument and the new count check still hold. #1299 and #1300 conflict only in docs/releases/v0.15.0.md. #1275 and #1282 also touch table_store.rs and merge cleanly with this head.
Liability
The delta lowers liability. The physical assumption that the previous head added is now checked at run time. The candidate/delete distinction is now enforced by the type system, so three run-time checks and two unit assertions are gone. The shared push_with_bounds and check_bounds keep a single accounting path. Item 2 above is the only new drift risk, and it is small.
Validation
Local runs on a Linux gVisor host, debug build, RUST_MIN_STACK=16777216 as in CI, through cargo dev-nextest in a clean detached worktree:
| Tree | Tests | Result |
|---|---|---|
a6334ad9 (head) |
merge_cost::merge_delete_selects_rows_by_rowid_issue_1295 (off, on, verify), lance_surface_guards::delete_chunk_reads_by_filter_form_issue_1295, chain_limit_tests::branch_merge_delete_ids_split_on_row_and_escaped_byte_bounds, chain_limit_tests::branch_merge_delete_chunks_chain_one_link_each, staged_tests::stage_delete_does_not_advance_head_and_reads_through_staged |
5 passed |
a6334ad9 with one temporary fault: DeleteIdChunk::delete_expr replaces the chunk's last _rowid with an id no row has |
merge_delete_selects_rows_by_rowid_issue_1295 |
the merge refuses, as intended: Unpublished, branch merge delete chunk 1 for table 'node:Person' removed 1 of its 2 rows |
4396d615 engine source (exec/merge.rs, storage_layer.rs, table_store.rs, staged_tests.rs) with the head's test and the same fault |
same | the merge publishes, then the row-count assertion fails: left: 68, right: 67. The previous head publishes a delete that did not happen. |
The fault shows the new check in both directions: the same stale _rowid is a silent wrong result before f62d3b94 and a typed refusal after it. I restored all temporary edits; the worktree is clean at a6334ad9.
GQT cannot express this. The defect needs a _rowid that no longer names its row, and no graph operation produces one on Lance 13. So the fault has to be injected at the engine layer, and the Rust _issue_1295 test is the right owner. The new stage_delete unit assertion pins Lance's count for a filter that matches only part of its list.
Exact-commit CI at a6334ad9: every check passes, including Test Workspace, GQ Logic Tests, GQT (ordinary, served, dst), DST pinned suite, Storage Upgrade Compatibility, both RustFS S3 jobs and Azurite. I did not rerun the full merge suite, GQT corpus or DST suite locally; those results come from CI and from the author's reported runs. This delta makes no performance claim.
…deletes The merge compares the staged delete's row count with the chunk's, not row identity. Say why a count is enough: stable row ids are never reused within a lineage, and only a target-side read supplies them. StagedWrite::deleted_rows is now set only by stage_delete, where Lance's delete scan counts the rows. stage_delete_rows deletes addresses the caller supplies, so it leaves the count unset instead of echoing the request.
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: ready. No blocking findings and no new optional notes.
This is a delta review. It covers head 711ef93be3ce0b98c0964bcd4d324a7e72d7d15e. My previous review covered a6334ad9. The delta is one commit, 711ef93b, +19/-18 in three files. It takes both optional notes from that review.
Why, what, how, impact
Why. The previous review made two small notes. The comment on the merge's delete-count check said more than a count comparison proves. And deleted_rows had two sources with one doc: Lance's own count after stage_delete, but the caller's requested count after stage_delete_rows.
What.
- The comment in
commit_staged_delete_chunks(merge.rs:4577-4583) now says that the check compares counts, not row identity. It also says why a count is enough: stable row ids are not reused within a lineage, and only a target-side read supplies them. So a stale_rowidcan only miss its row. stage_delete_rowsno longer setsdeleted_rows(table_store.rs:3458-3463). The field is nowSomeonly when Lance's delete scan counted the rows. The field doc and both accessor docs say this, and namestage_delete_rowsas aNonecase.
How I checked it.
num_deleted_rowsinstage_delete_rowsis still used for the empty-address no-op check (table_store.rs:3418-3421). So the change leaves no unused variable.- The only production reader of
deleted_rows()is the merge check (merge.rs:4585-4591). It callsstage_delete, neverstage_delete_rows. The mutation path (exec/staging.rs:1004) callsstage_delete_rows, orstage_deletewhen it has no addresses at the same version, and reads the field in neither case. So no current behavior changes. A future caller that reads the field afterstage_delete_rowsnow getsNone(and the merge check would refuse it) instead of an unchecked count. That is the safe default. - The comment's premise is a documented invariant: "Graph tables use stable row IDs" (
docs/dev/lance.md:173). Tables are created withenable_stable_row_ids: true(table_store.rs:3329). - The two unit assertions on
deleted_rows()(staged_tests.rs:3235,:3279) both usestage_delete, so they still test the checked source.
Impact. The comment and doc only change. One field is no longer set on a path that no code reads it from. No API, on-disk state or format change. No performance effect.
Blocking
None.
Optional
None new. Both earlier optional notes are resolved as I suggested. The note for whoever merges second still applies: this PR, #1272 and #1235 each append a guard at the end of crates/omnigraph/tests/lance_surface_guards.rs, so git merge-tree shows a text conflict there. Keep both sides. #1299 and #1300 still conflict with this PR only in docs/releases/v0.15.0.md.
Alignment with other open work
No change from my previous review. #1275, #1282 and #1291 merge cleanly with this head. No other open PR reads StagedWrite::deleted_rows, so the narrower contract affects only this PR.
Liability
The delta lowers liability a little. One field now has one meaning and one source, so it cannot drift between two writers. The comment no longer invites a reader to trust the check for row identity. Nothing is added.
Validation
Local run on a Linux gVisor host, debug build, RUST_MIN_STACK=16777216 as in CI, through cargo dev-nextest in a clean detached worktree at 711ef93b. The build had no compiler warnings.
| Test | Result |
|---|---|
merge_cost::merge_delete_selects_rows_by_rowid_issue_1295 (off, on, verify) |
pass |
lance_surface_guards::delete_chunk_reads_by_filter_form_issue_1295 |
pass |
chain_limit_tests::branch_merge_delete_ids_split_on_row_and_escaped_byte_bounds |
pass |
chain_limit_tests::branch_merge_delete_chunks_chain_one_link_each |
pass |
staged_tests::stage_delete_does_not_advance_head_and_reads_through_staged |
pass |
I did not repeat the fault injection from my previous review. This delta does not change delete_expr, the count check or stage_delete, so that before/after result still holds.
Exact-commit CI at 711ef93b, read before posting: Test Workspace, Lint (clippy), GQT (ordinary, served, dst, dst-clippy), DST pinned suite, both RustFS S3 jobs and Azurite pass. Storage Upgrade Compatibility was still running. I did not rerun the full merge suite, GQT corpus or DST suite locally. This delta makes no performance claim.
GQT: this delta fixes no behavior, so no new regression test applies. The PR's _issue_1295 Rust test stays the right owner, for the reason my previous review gave: no graph operation produces a stale _rowid on Lance 13.
|
Thanks for the delta review of
Noted on the Local checks
Base main |
…-rowid # Conflicts: # docs/releases/v0.15.0.md
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: ready. No blocking findings and no new optional notes.
This is a delta review. It covers head a04deb7f611c0b8e014196ee2208e6e151773acc. My previous review covered 711ef93b. The delta is two commits: 099aac3f, a merge of main at ea4a16d5 (#1299), and a04deb7f, which regenerates the v0.15.0 release snapshot.
Why, what, how, impact
Why. #1299 landed on main, and it also edits docs/releases/v0.15.0.md. My previous review said this PR conflicted with #1299 only in that file.
What.
- The merge takes
mainfor the conflicted snapshot header. Its only other text change is that it drops this PR's line from the snapshot (git show --remerge-diff 099aac3f). No source or test file needed a manual resolution. a04deb7fregenerates the snapshot. It puts this PR's line back (v0.15.0.md:152). It also adds the #1299 link to thenearest()line (v0.15.0.md:119), whichmaindid not have yet.
How I checked it.
- The PR's own diff is unchanged by the merge.
git diff 2d8effab 711ef93bandgit diff ea4a16d5 a04deb7f, both without the release file, differ only in four hunk line offsets intable_store.rs(+5 and +11 lines, from #1299's additions above them). - #1299 and this PR touch different code. #1299 changes
nearestranking (ScanTuning::nearest,execute_node_scan) and the vector-index fold (vector_partition_count). This PR changes the merge delete path (exec/merge.rs,StagedWrite::deleted_rows,stage_delete). They sharetable_store.rsbut no function, and no caller of one reaches the other. python3 scripts/release_notes.py verify --version v0.15.0 --target HEADin the pinned docs environment:Release notes OK: v0.15.0, 60bfd19d..a04deb7f.- The head contains current
main(ea4a16d5).
Impact. No code change in the delta. The release snapshot now lists both #1299 and this PR.
Blocking
None.
Optional
None new. The merge note from my previous review still applies with one update. The #1299 conflict is gone. git merge-tree against the current heads shows:
- #1272 and #1235: text conflict at the end of
crates/omnigraph/tests/lance_surface_guards.rs, where each PR appends a guard. Keep both sides. - #1300: conflict only in
docs/releases/v0.15.0.md. Whichever merges second regenerates the snapshot. - #1291 merges cleanly. #1275 and #1282 conflict in
crates/omnigraph/tests/search.rs, but they conflict withmainthe same way, so the cause is #1299, not this PR.
Alignment with other open work
No change from my previous review.
Liability
No change. The delta adds no API, state or invariant.
Validation
Local run on a Linux gVisor host, debug build, RUST_MIN_STACK=16777216 as in CI, through cargo dev-nextest in a clean detached worktree at a04deb7f:
The build had no compiler warnings. 12 tests selected, 12 pass:
| Test | Result |
|---|---|
merge_cost::merge_delete_selects_rows_by_rowid_issue_1295 |
pass |
lance_surface_guards::delete_chunk_reads_by_filter_form_issue_1295 |
pass |
chain_limit_tests::branch_merge_delete_ids_split_on_row_and_escaped_byte_bounds |
pass |
chain_limit_tests::branch_merge_delete_chunks_chain_one_link_each |
pass |
staged_tests::stage_delete_does_not_advance_head_and_reads_through_staged |
pass |
merge_truth_table::merge_pair_truth_table, add_edge_add_edge_keyed_twin_converges |
pass |
server and CLI branch_merge_delete_branch_* (5 tests) |
pass |
Exact-commit CI at a04deb7f, read before posting: all 27 jobs that ran pass, including Test Workspace, Lint (clippy), GQT (ordinary, served, dst, dst-clippy), DST pinned suite, Fix Regression Gate, Storage Upgrade Compatibility, both RustFS S3 jobs and Azurite. Four jobs are skipped: the Graph Vocabulary Guard and three tag-publishing jobs that depend on it.
I did not rerun the full merge suite, GQT corpus or DST suite locally. This delta makes no performance claim.
GQT: the delta fixes no behavior, so no new regression test applies. The _issue_1295 Rust test stays the right owner, for the reason given in my earlier reviews.
…-rowid # Conflicts: # docs/releases/v0.15.0.md
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: ready. No blocking findings and no new optional notes.
This is a delta review. It covers head 5cf0c28e8f783c47efdae240a30622f144c1f31e. My previous review covered a04deb7f. The delta is one commit: 5cf0c28e, a merge of main at 14753b56 (#1300).
Why, what, how, impact
Why. #1300 landed on main. It also edits docs/releases/v0.15.0.md, so this PR had a conflict in that file, as my previous review predicted.
What.
- The merge has one manual resolution: the v0.15.0 snapshot (
git show --remerge-diff 5cf0c28e). It takes a regenerated header and keeps both lines: #1300's line with its link (v0.15.0.md:152) and this PR's line (v0.15.0.md:153). - No source or test file needed a manual resolution.
How I checked it.
- The PR's own diff did not change. I compared
git diff ea4a16d5 a04deb7fwithgit diff 14753b56 5cf0c28e. Outside the release file, they differ only in hunk line offsets intable_store.rs,staged_tests.rsandlance_surface_guards.rs, which come from #1300's additions above them. - #1300 and this PR touch different code. #1300 changes how index coverage is judged (
dataset_index::SegmentCoverage, used by the read planner's index facts inengine/plan_source/indexes.rsand byTableStore::foldable_index_lag). This PR changes the merge delete path (exec/merge.rs,StagedWrite::deleted_rows,stage_delete). They sharetable_store.rs, but no function. The Lance surface guarddelete_chunk_reads_by_filter_form_issue_1295checks Lance's scanner directly, so Omnigraph's coverage rule does not change what it measures. - I regenerated the snapshot with
python3 scripts/release_notes.py snapshot --target HEAD --date 2026-11-06 --replacein the pinned docs environment. The only difference from the committed file is the header'stargetrevision, which names the commit the snapshot was made from. I restored the file afterwards. python3 scripts/release_notes.py verify --version v0.15.0 --target HEAD:Release notes OK: v0.15.0, 60bfd19d..5cf0c28e.- The head contains current
main(14753b56), and it merges cleanly into it.
Impact. No code change in the delta. The release snapshot now lists both #1300 and this PR.
Blocking
None.
Optional
None new. Merge notes, updated with git merge-tree against the current open heads:
- #1272 (
8718823a) and #1235: text conflict at the end ofcrates/omnigraph/tests/lance_surface_guards.rs, where each PR adds a guard. Keep both sides. #1272 also conflicts in the release snapshot. - #1304 and #1184: conflict only in
docs/releases/v0.15.0.md. Whichever merges second regenerates the snapshot. - #1291, #1275 and #1282 merge cleanly.
Alignment with other open work
No change from my previous review. One note on #1272's new head: its column patch "moves no row, so each row keeps its stable row id", and it does not touch exec/merge.rs. So it does not weaken this PR's argument that a _rowid read at the target pin still names the same row when the merge's deletes run.
Liability
No change. The delta adds no API, state or invariant.
Validation
Local run on a Linux gVisor host, debug build, RUST_MIN_STACK=16777216 as in CI, in a clean detached worktree at 5cf0c28e:
cargo dev-nextest -E 'test(issue_1295) | test(/merge::.*delete_id_chunk/) | test(/stage_delete/) | (package(omnigraph-engine) & test(/merge/))' --no-fail-fast
The build had no compiler warnings. 177 tests selected, 177 pass, 0 fail. They include:
| Test | Result |
|---|---|
merge_cost::merge_delete_selects_rows_by_rowid_issue_1295 |
pass |
lance_surface_guards::delete_chunk_reads_by_filter_form_issue_1295 |
pass |
chain_limit_tests::branch_merge_delete_ids_split_on_row_and_escaped_byte_bounds |
pass |
chain_limit_tests::branch_merge_delete_chunks_chain_one_link_each |
pass |
staged_tests::stage_delete_does_not_advance_head_and_reads_through_staged |
pass |
staged_tests::stage_delete_commit_rebases_over_disjoint_committed_delete |
pass |
merge_truth_table::merge_pair_truth_table, add_edge_add_edge_keyed_twin_converges |
pass |
| the rest of the engine's merge tests | pass |
Exact-commit CI at 5cf0c28e, read before posting: all 28 jobs that ran pass, including Test Workspace, Lint (clippy), GQT (ordinary, served, dst, dst-clippy), DST pinned suite, Fix Regression Gate, Storage Upgrade Compatibility, both RustFS S3 jobs and Azurite. Four jobs are skipped: the Graph Vocabulary Guard and three tag-publishing jobs that depend on it.
I did not rerun the GQT corpus or the DST suite locally. This delta makes no performance claim.
GQT: the delta fixes no behavior, so no new regression test applies. The _issue_1295 Rust test stays the right owner, for the reason given in my earlier reviews.
…-rowid # Conflicts: # crates/omnigraph/tests/lance_surface_guards.rs # docs/releases/v0.15.0.md
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: ready. No blocking findings and no new optional notes.
This is a delta review. It covers head 8a1052f0615d1b36a5ab4713b7f7f97ca9f06825. My previous review covered 5cf0c28e. The delta is two commits: 793e4e93, a merge of main at e7e8e03b (#1272), and 8a1052f0, which regenerates the v0.15.0 release snapshot.
Why, what, how, impact
Why. #1272 landed on main. As my previous review predicted, it conflicted with this PR at the end of crates/omnigraph/tests/lance_surface_guards.rs and in docs/releases/v0.15.0.md.
What.
lance_surface_guards.rs: the resolution keeps both guards. This PR'sdelete_chunk_reads_by_filter_form_issue_1295comes first, then #1272'scolumn_patch_keeps_blob_files_row_ids_and_key_index_coverage_issue_1264(git show --remerge-diff 793e4e93). No line of either guard changed.docs/releases/v0.15.0.md:8a1052f0regenerates the snapshot. It keeps this PR's line and #1272's line, which now has its PR link, and moves the header'stargetto793e4e93.- No source file needed a manual resolution.
How I checked it.
- The PR's own diff did not change. For every file except the two above, the changed lines of
git diff 14753b56 5cf0c28eandgit diff e7e8e03b 8a1052f0are identical (same hash of the+/-lines):exec/merge.rs,storage_layer.rs,table_store.rs,staged_tests.rs,merge_cost.rs,docs/dev/merge.mdand the changelog entry. Inlance_surface_guards.rsthe only difference is where one blank line sits at the join with #1272's guard. - I checked the one new interaction: #1272's column patch and this PR's delete by
_rowid. A patch (RewriteColumns) keeps each row's fragment and stable row id (docs/dev/writes.md, "Column-patch updates";docs/dev/lance.md, "Column patch"). So a_rowidthat the merge reads at the target pin after a patch still selects the same row. I checked this with a test (see Validation). - The head contains current
main(e7e8e03b). GitHub reports itMERGEABLE/CLEAN.
Impact. No code change in the delta. The release snapshot now lists #1272 and this PR.
Blocking
None.
Optional
None new. Merge notes, updated with git merge-tree against the current open heads:
- #1235 (draft,
1a856292): text conflict at the end ofcrates/omnigraph/tests/lance_surface_guards.rs, where each PR adds a guard. Keep both sides. - #1184 (
50e3b863): conflict only indocs/releases/v0.15.0.md. Whichever merges second regenerates the snapshot. - #1304 (
372c7ada): conflicts indocs/releases/v0.15.0.mdand in a doc comment abovecopy_batch_rangeintable_store.rs. That second conflict comes from #1272, which is now onmain, and #1304 has not mergedmainyet. This PR does not touch that function. - #1291, #1282, #1275 and #1302 merge cleanly.
Alignment with other open work
No change from my previous review. #1272 is now on main, so the point I noted last time is now in the code: a column patch moves no row and keeps its stable row id, so a _rowid that the merge reads at the target pin still names the same row when the merge's delete runs.
Liability
No change. The delta adds no API, state or invariant.
Validation
Local run on a Linux gVisor host, debug build, RUST_MIN_STACK=16777216 as in CI, in a clean detached worktree at 8a1052f0. cargo dev-build-tests built with no compiler warnings.
- The same selection as my previous review, plus #1272's tests:
cargo dev-nextest -E 'test(issue_1295) | test(/merge::.*delete_id_chunk/) | test(/stage_delete/) | (package(omnigraph-engine) & test(/merge/)) | test(issue_1264)' --no-fail-fast
181 tests pass, 0 fail. They includemerge_cost::merge_delete_selects_rows_by_rowid_issue_1295,lance_surface_guards::delete_chunk_reads_by_filter_form_issue_1295,chain_limit_tests::branch_merge_delete_ids_split_on_row_and_escaped_byte_bounds, thestage_deletetests, the engine's merge tests,merge_truth_table::merge_pair_truth_table, and #1272'supdate_of_one_scalar_patches_instead_of_rewriting_blobs_and_vectors_issue_1264,column_patch_keeps_blob_files_row_ids_and_key_index_coverage_issue_1264andchanges_cost::diff_of_a_column_patch_commit_takes_row_identity_issue_1264. cargo dev-nextest -E 'binary(lance_surface_guards) | binary(forbidden_apis)': 113 pass (74 guards, 39 forbidden-API checks), 2 skipped by their own#[ignore].- A temporary composition test (not for inclusion). Each row of a
Doctype has a 64 KiB Blob and aVector(8), and all 8 rows are in one fragment.featuredeletesd3andd5.mainupdates one scalar ond2, and the test asserts that this update stagedUpdateMode::RewriteColumns. Then it mergesfeatureintomain. In eachmerge_lineagemode (off,on,verify), the merge's last Person transaction is aDeletewhose predicate starts with_rowid IN (. 6 rows remain,d3andd5are gone,d2keepsscore = 1, and every remaining row keeps its vector. Result: pass. I removed the test afterwards. The worktree is clean. - Release notes, in the pinned docs environment:
python3 scripts/release_notes.py verify --version v0.15.0 --target HEADgivesRelease notes OK: v0.15.0, 60bfd19d..8a1052f0. When I regenerated the snapshot withsnapshot --target HEAD --date 2026-11-06 --replace, the only difference was the header'stargetrevision. I restored the file.
Exact-commit CI at 8a1052f0, read before posting: all 27 jobs that ran pass, including Test Workspace, Lint (clippy), GQT (ordinary, served, dst, dst-clippy), DST pinned suite, Fix Regression Gate, Storage Upgrade Compatibility, both RustFS S3 jobs, Azurite and the CI Gate. Four jobs are skipped: the Graph Vocabulary Guard and three tag-publishing jobs that depend on it.
I did not rerun the GQT corpus or the DST suite locally. This delta makes no performance claim.
GQT: the delta fixes no behavior, so no new regression test applies. The _issue_1295 Rust test stays the right owner, for the reason given in my earlier reviews.
…-rowid # Conflicts: # docs/releases/v0.15.0.md
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: ready. No blocking findings and no new optional notes.
This is a delta review. It covers head 12c2bbebad5a3cbcaf629f2c0770bd7fa2677092. My previous review covered 8a1052f0. The delta is two commits: ed35a1ed, a merge of main at 0b468487 (#1304), and 12c2bbeb, which regenerates the v0.15.0 release snapshot.
Why, what, how, impact
Why. #1304 landed on main. It edits crates/omnigraph/src/table_store.rs and docs/releases/v0.15.0.md, which this PR also edits. As my previous review predicted, the release snapshot conflicted.
What.
- The merge has one manual resolution:
docs/releases/v0.15.0.md(git show --remerge-diff ed35a1ed). No source or test file needed a manual resolution. 12c2bbebregenerates the snapshot. Compared with8a1052f0, the header gains #1304's changelog entry (link and digest) and movestargettoed35a1ed. The body gains #1304's line. This PR's line does not change.
How I checked it.
- The PR's own diff did not change. Outside
docs/releases/v0.15.0.md,git diff e7e8e03b 8a1052f0andgit diff 0b468487 12c2bbebare identical onceindexlines are dropped, in all 8 files:exec/merge.rs,storage_layer.rs,table_store.rs,staged_tests.rs,merge_cost.rs,lance_surface_guards.rs,docs/dev/merge.mdand the changelog entry. - I checked the one new interaction, #1304 against this PR in
table_store.rs. #1304 makesPendingScanAccount::retainthe only batch charge of the pending-aware update scans (retain). Onlyscan_with_pending,scan_with_pending_materialized_blobsandscan_update_matchescreate that account. Their callers outsidetable_store.rsareexec/mutation.rsandexec/blob_write.rs.exec/merge.rscalls none of them. Its delete goes throughstage_delete(merge.rs:4569), which #1304 does not touch. The two PRs share the file but no function or state. - The head contains current
main(0b468487). GitHub reports itMERGEABLE/CLEAN.
Impact. No code change in the delta. The release snapshot now lists #1304 and this PR.
Blocking
None.
Optional
None new. Merge notes, from git merge-tree against the current open heads:
- #1235 (draft,
1a856292): text conflict at the end ofcrates/omnigraph/tests/lance_surface_guards.rs, where each PR adds a guard. Keep both sides. - #1184 (
c36bf618) and #1175 (draft,4c93197b): conflict only indocs/releases/v0.15.0.md. Whichever merges second regenerates the snapshot. - #1302, #1291, #1282 and #1275 merge cleanly.
Alignment with other open work
No change from my previous review. #1304 changes how an update is admitted under write_max_bytes. It does not change which rows a merge reads or deletes, so this PR's argument still holds: a _rowid read at the target pin names the same row when the merge's delete runs.
Liability
No change. The delta adds no API, state or invariant.
Validation
Local run on a Linux gVisor host, debug build, RUST_MIN_STACK=16777216 as in CI, in a clean detached worktree at 12c2bbeb. cargo dev-build-tests built with no compiler warnings.
- The same selection as my previous review, plus #1304's name:
cargo dev-nextest -E 'test(issue_1295) | test(/merge::.*delete_id_chunk/) | test(/stage_delete/) | (package(omnigraph-engine) & test(/merge/)) | test(issue_1264) | test(issue_1303)' --no-fail-fast
181 tests pass, 0 fail. They includemerge_cost::merge_delete_selects_rows_by_rowid_issue_1295,lance_surface_guards::delete_chunk_reads_by_filter_form_issue_1295,chain_limit_tests::branch_merge_delete_ids_split_on_row_and_escaped_byte_bounds, thestage_deletetests,merge_truth_table::merge_pair_truth_tableand the_issue_1264tests. #1304's regression is a GQT case, which this selection does not run (see the CI result below). cargo dev-nextest -E 'binary(lance_surface_guards) | binary(forbidden_apis)': 113 pass, 2 skipped by their own#[ignore].- Release notes, in the pinned docs environment:
python3 scripts/release_notes.py verify --version v0.15.0 --target HEADgivesRelease notes OK: v0.15.0, 60bfd19d..12c2bbeb. When I regenerated the snapshot withsnapshot --target HEAD --date 2026-11-06 --replace, the only difference was the header'stargetrevision. I restored the file. The worktree is clean.
Exact-commit CI at 12c2bbeb, read before posting: every job that ran passes, including Test Workspace, Lint (clippy), GQT (ordinary, served, dst, dst-clippy), DST pinned suite, Fix Regression Gate, Storage Upgrade Compatibility, both RustFS S3 jobs, Azurite and the CI Gate. The GQT jobs include #1304's issue_1303_update_admission_ignores_fragment_neighbours case. Four jobs are skipped: the Graph Vocabulary Guard and three tag-publishing jobs that depend on it.
I did not rerun the GQT corpus or the DST suite locally. This delta makes no performance claim.
GQT: the delta fixes no behavior, so no new regression test applies. The _issue_1295 Rust test stays the right owner, for the reason given in my earlier reviews.
…-rowid # Conflicts: # docs/releases/v0.15.0.md
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: ready. No blocking findings and no new optional notes.
This is a delta review. It covers head 50bb8380c124bb79193c86446d679a761698e7c0. My previous review covered 12c2bbeb. The delta is two commits: a22f0c84, a merge of main at f0df4673 (#1307), and 50bb8380, which regenerates the v0.15.0 release snapshot.
Why, what, how, impact
Why. #1307 landed on main. It edits docs/releases/v0.15.0.md, which this PR also edits, so the release snapshot conflicted again.
What.
- The merge has one manual resolution:
docs/releases/v0.15.0.md(git show --remerge-diff a22f0c84). No source or test file needed a manual resolution. 50bb8380regenerates the snapshot. Compared withmainatf0df4673, the header adds this PR's changelog digest and movestargettoa22f0c84, and the body adds this PR's line. Compared with12c2bbeb, the changes all come frommain: the release date (2026-11-09) and #1293's reworded macOS line and its digest. This PR's line does not change.
How I checked it.
- The PR's own diff did not change. Outside
docs/releases/v0.15.0.md,git diff 0b468487 12c2bbebandgit diff f0df4673 50bb8380are identical onceindexlines are dropped, in all 8 files:exec/merge.rs,storage_layer.rs,table_store.rs,staged_tests.rs,merge_cost.rs,lance_surface_guards.rs,docs/dev/merge.mdand the changelog entry. - #1307 has no interaction with this PR. It changes
.github/workflows/build-release-archives.yml(notarization status read fromnotarytool info),docs/user/install.md,docs/releases/README.mdand its own changelog entry. The only shared file is the release snapshot. - The head contains current
main(f0df4673). GitHub reports itMERGEABLE.
Impact. No code change in the delta. The release snapshot now matches main plus this PR's line.
Blocking
None.
Optional
None new. Merge notes, from git merge-tree against the current open heads (I list only files this PR edits):
- #1306 (
40075781): new conflict, only indocs/releases/v0.15.0.md. It merges cleanly withmain, so whichever of the two merges second regenerates the snapshot. - #1184 (
c36bf618), #1175 (draft,4c93197b) and #952 (00ad7927): conflict indocs/releases/v0.15.0.md, as they already do againstmain. - #1235 (draft,
1a856292) and #785 (0069e925): conflict at the end ofcrates/omnigraph/tests/lance_surface_guards.rs, as they already do againstmain. Keep both sides.
Alignment with other open work
No change from my previous review. #1307 is release CI only. It does not change which rows a merge reads or deletes, so this PR's argument still holds: a _rowid read at the target pin names the same row when the merge's delete runs.
Liability
No change. The delta adds no API, state or invariant.
Validation
Local run on a Linux gVisor host, debug build, RUST_MIN_STACK=16777216 as in CI, in a clean detached worktree at 50bb8380.
cargo dev-build-tests built with no compiler warnings.
- The same selection as my previous review:
cargo dev-nextest -E 'test(issue_1295) | test(/merge::.*delete_id_chunk/) | test(/stage_delete/) | (package(omnigraph-engine) & test(/merge/)) | test(issue_1264) | test(issue_1303)' --no-fail-fast
181 tests pass, 0 fail. They includemerge_cost::merge_delete_selects_rows_by_rowid_issue_1295andlance_surface_guards::delete_chunk_reads_by_filter_form_issue_1295. cargo dev-nextest -E 'binary(lance_surface_guards) | binary(forbidden_apis)': 113 pass, 2 skipped by their own#[ignore].
Release notes and docs, in the pinned docs environment (scripts/requirements-docs.txt):
python3 scripts/release_notes.py verify --version v0.15.0 --target HEADgivesRelease notes OK: v0.15.0, 60bfd19d..50bb8380.- When I regenerated the snapshot with
snapshot --version v0.15.0 --target HEAD --date 2026-11-09 --replace, the only difference was the header'stargetrevision. I restored the file. The worktree is clean. check-docs.pyOK (473 files);test_release_notes.pyOK;check-release-note.py --self-test21/21;git diff --check f0df4673 HEADclean.
Exact-commit CI at 50bb8380, read before posting: every job that ran passes (29), including Test Workspace, Lint (clippy), the GQT and DST jobs, Storage Upgrade Compatibility, the RustFS S3 and Azurite jobs and the CI Gate. Four jobs are skipped: the Graph Vocabulary Guard and three tag-publishing jobs that depend on it.
I did not rerun the GQT corpus or the DST suite locally. This delta makes no performance claim.
GQT: the delta fixes no behavior, so no new regression test applies. The _issue_1295 Rust test stays the right owner, for the reason given in my earlier reviews.
Closes #1295. A branch merge staged each delete chunk as
id IN (...). Lance runs that filter through its scanner, which searches theidBTREE and then scans theidcolumn of every fragment the index does not cover, once per chunk. The rows a merge deletes were already read by the merge walk,_rowidincluded. Reproduced on main2d8effabe1900f63c30ecbf918c8fcfe3080f025. With the scanDeleteJobbuilds, measured by Lance'sanalyze_planon a 2,000-row-per-fragment table whose BTREE covers only the first fragment, oneid INdelete read 8,000 rows, 124.6 KB and 12 I/O operations with 4 uncovered fragments, and 32,000 rows, 510.3 KB and 48 I/O operations with 16. The cost grows with the uncovered tail and does not depend on how many rows the chunk deletes.Each delete chunk now keeps the
_rowidof every row it removes, beside its id, and stages_rowid IN (...)(DeleteIdChunk::delete_exprinexec/merge.rs). Lance'sTakeOperation::try_from_exprturns this filter into a take by row id (lance-13.0.0src/dataset/scanner.rs:1337-1427, 3326-3345): one row read per deleted row, with no index and no data column. The ids still go torecord_deleted_idsfor the change feed, and they still drive the byte plan the lineage candidate chunks share. The guard is where the_rowidcomes from: it must be a row of the table the delete applies to. The three-way and lineage paths take it from the target cursor row. The adopt path takes it from the base cursor row, which is valid only because adopt runs only when the target's table entry equals the base's (same_manifest_state: same path, version and native branch). Source-side row ids are never used, because sibling detached commits can give the same new_rowidto different rows. Delete chunks (DeleteIdChunks: id and_rowid) and lineage candidate chunks (CandidateIdChunks: ids only) are two types of oneIdChunks<R>, so a candidate chunk cannot build a delete. Each chunk's delete must remove exactly the chunk's rows, as Lance counts them while staging; otherwise the merge fails before the chunk's change record is committed. Verify mode now compares the deleted (id,_rowid) pairs of the two classifiers.The test
merge_cost::merge_delete_selects_rows_by_rowid_issue_1295runs a three-way merge that deletes two rows, withmerge_lineageset tooff(full walk),on(lineage) andverify. All rows start in one fragment; the source also updates the rows next to the deleted ones and the target updates another, so the update link rewrites the fragment the delete then takes from. It reads the merge's Delete transaction and checks which rows were deleted and kept, and the updated values. On main it fails: Lance recorded the filter__id IN ([Utf8("dv-0"), Utf8("dv-63")]). On this branch the filter is_rowid IN (...). The Lance surface guarddelete_chunk_reads_by_filter_form_issue_1295pins the shortcut at the pinned Lance: the_rowid INscan has no filter and reads 1 row, 0 bytes and 0 I/O operations for 4 and 16 uncovered fragments, whileid INscans at least every uncovered fragment. After the change the same delete reads 1 row, 0 bytes and 0 I/O operations at both sizes. The chunk unit test now also checks the expression shape and the (id,_rowid) order, and astage_deleteunit test checks that a partly matching filter reports the rows Lance actually removed. Local runs: cargo fmt; the in-source merge tests; nextest of every engine test matching merge or branch (309, all pass with CI'sRUST_MIN_STACK; without it five debug-build tests overflow their stack, the class CI's floor exists for); and the GQT corpus (462 cases). The DST suite also passes locally, with the storage-cost golden unchanged. After the review follow-up (f62d3b94,a6334ad9): the focused #1295 tests, the 309 engine merge and branch tests, the GQT corpus (462), the DST pinned suite (cost golden unchanged), workspace clippy and cargo fmt pass again.For review: the decision to challenge is that a
_rowidread at the target pin still names the same row after the merge's own insert and update links commit, because the deletes run last in the chain. Inserts get new row ids from the target'snext_row_id. Updates rewrite only rows the merge keeps. With stable row ids the id survives a rewrite. On a legacy table without stable row ids,_rowidis the row address, and an earlier link adds deletion vectors and new fragments but does not move the rows a delete names. The input that would expose a mistake is a table where aKnownPresentUpdatelink rewrites a fragment holding a row the same merge deletes. If that argument were wrong, the delete would remove fewer rows than its chunk holds, and the row-count check stops the merge instead of publishing a change record for rows that were not deleted. I chose_rowidrather than deleting by the captured address throughstage_delete_rows, as mutations do: an address is valid only at the version it was read, and the merge chain has moved past that version when its deletes run. The transaction's recorded predicate text changes from ids to row ids. Nothing in Omnigraph reads it back; the change feed readsrecord_deleted_ids. Storage-cost note: an object-storeIOTrackeraround the dataset did not see the scanner's data reads (it reported one 823-byte read for deletes that must read every uncovered fragment), so object-store counts may understate scan costs; that is why the evidence usesanalyze_plan. No public API, file format or documented behaviour changes. The merge developer guide and a performance release note are updated. The regenerated v0.15.0 release snapshot will conflict with the ones in #1299 and #1300; whichever lands later regenerates it.Lance analyze_plan, one delete, BTREE over the first fragment only
Base: main
2d8effabe1900f63c30ecbf918c8fcfe3080f025; pinned Lance 13.0.0. Debug build, gVisor host.Workflow run: run-wf-reproduce-modernrelay-omnigraph-issue-20261107T1812Z-bceded