Skip to content

perf(engine): delete a branch merge's rows by the _rowid its walk read - #1301

Merged
ragnorc merged 14 commits into
ModernRelay:mainfrom
ragnorc:fix/1295-merge-delete-rowid
Nov 9, 2026
Merged

ragnorc merged 14 commits into
ModernRelay:mainfrom
ragnorc:fix/1295-merge-delete-rowid

Conversation

@ragnorc

@ragnorc ragnorc commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1295. A branch merge staged each delete chunk as id IN (...). Lance runs that filter through its scanner, which searches the id BTREE and then scans the id column of every fragment the index does not cover, once per chunk. The rows a merge deletes were already read by the merge walk, _rowid included. Reproduced on main 2d8effabe1900f63c30ecbf918c8fcfe3080f025. With the scan DeleteJob builds, measured by Lance's analyze_plan on a 2,000-row-per-fragment table whose BTREE covers only the first fragment, one id IN delete 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 _rowid of every row it removes, beside its id, and stages _rowid IN (...) (DeleteIdChunk::delete_expr in exec/merge.rs). Lance's TakeOperation::try_from_expr turns this filter into a take by row id (lance-13.0.0 src/dataset/scanner.rs:1337-1427, 3326-3345): one row read per deleted row, with no index and no data column. The ids still go to record_deleted_ids for the change feed, and they still drive the byte plan the lineage candidate chunks share. The guard is where the _rowid comes 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 _rowid to different rows. Delete chunks (DeleteIdChunks: id and _rowid) and lineage candidate chunks (CandidateIdChunks: ids only) are two types of one IdChunks<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_1295 runs a three-way merge that deletes two rows, with merge_lineage set to off (full walk), on (lineage) and verify. 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 guard delete_chunk_reads_by_filter_form_issue_1295 pins the shortcut at the pinned Lance: the _rowid IN scan has no filter and reads 1 row, 0 bytes and 0 I/O operations for 4 and 16 uncovered fragments, while id IN scans 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 a stage_delete unit 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's RUST_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 _rowid read 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's next_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, _rowid is 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 a KnownPresentUpdate link 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 _rowid rather than deleting by the captured address through stage_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 reads record_deleted_ids. Storage-cost note: an object-store IOTracker around 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 uses analyze_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
uncovered=4  id IN     LanceRead full_filter=id = Utf8("tail0-5") fragments_scanned=4  rows_scanned=8.00 K  bytes_read=124.6 K iops=12
uncovered=4  _rowid IN LanceRead full_filter=--                    fragments_scanned=1  rows_scanned=1      bytes_read=0       iops=0
uncovered=16 id IN     LanceRead full_filter=id = Utf8("tail0-5") fragments_scanned=16 rows_scanned=32.00 K bytes_read=510.3 K iops=48
uncovered=16 _rowid IN LanceRead full_filter=--                    fragments_scanned=1  rows_scanned=1      bytes_read=0       iops=0

Base: main 2d8effabe1900f63c30ecbf918c8fcfe3080f025; pinned Lance 13.0.0. Debug build, gVisor host.

Workflow run: run-wf-reproduce-modernrelay-omnigraph-issue-20261107T1812Z-bceded

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 ragnorc 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.

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.
  • KnownPresentUpdate runs merge_insert with UpdateAll and full rows, so it takes the RewriteRows path (lance-13.0.0/src/dataset/write/merge_insert.rs, the else arm 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-schema RewriteColumns path (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 _rowid from the base row. That is valid because adopt-with-delta runs only when same_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:253 ignores 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

  1. 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. With id 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_uncommitted already returns num_deleted_rows, and stage_delete sees it (table_store.rs:3343). Compare it with chunk.row_ids.len() and fail with a typed internal error when they differ. Cost: one integer comparison and a small change to what stage_delete returns. 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.

  2. Make the two chunk kinds two types. DeleteIdChunks now holds two kinds of chunk: lineage candidate chunks (ids only) and delete chunks (ids and _rowids). An empty row_ids vector 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 in delete_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 the id 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.

  3. Test the case the description calls risky. The description says the input that would expose a mistake is a merge whose KnownPresentUpdate link rewrites a row in the same fragment as a row the same merge deletes. merge_delete_selects_rows_by_rowid_issue_1295 deletes 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 deletes dv-0 and dv-63 and updates dv-1 and dv-62 (all in one fragment). The target updates dv-2 and inserts a row. The test checks row counts and values for merge_lineage = off, on and verify. 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_age on a source row next to the deleted ones, and one on the target. It would also be useful to add verify to 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 _rowid argument, 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 after ExprSimplifier) would bring back the scan, not a wrong result. delete_chunk_reads_by_filter_form_issue_1295 catches 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.
@ragnorc

ragnorc commented Nov 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review of 4396d615. I took all three optional items. New head: a6334ad9 (f62d3b94 for items 1 and 2, a6334ad9 for item 3).

  1. A partial match now fails closed. stage_delete keeps the count of rows Lance deleted (num_deleted_rows from execute_uncommitted) on the staged write. commit_staged_delete_chunks compares it with the chunk's row count. If they differ, the merge stops with an internal error before the chunk's change record is written or committed. So a _rowid that no longer names its live row now fails loudly. It can no longer publish a record of a delete that did not happen. A stage_delete unit test checks that the count is what Lance actually removed for a filter that matches only part of its list.

  2. The two chunk kinds are now two types. IdChunks<R> holds one R per id: DeleteIdChunks = IdChunks<u64> (id and _rowid) and CandidateIdChunks = IdChunks<()> (ids only). delete_expr exists only on a delete chunk, so the mixed-chunk refusal, the length check in delete_expr and their two unit assertions are gone. Row, filter-byte and retained-byte accounting is shared and gives the same numbers as before (size_of::<()>() is 0). The doc comment now gives one reason per bound. The row bound exists because one chunk is one Lance transaction. The filter-byte bound is the string a candidate chunk sends to Lance; for a delete chunk it caps the ids in the change record and keeps the planned chunk count the same. The retained-byte bound caps the whole plan's heap before the first effect.

  3. The risky case is in the suite. merge_delete_selects_rows_by_rowid_issue_1295 now runs off, on and verify. The source deletes dv-0 and dv-63 and updates dv-1 and dv-62. The target updates dv-2 and inserts a row. All 64 rows start in one fragment. The test checks the Delete filter form, the row count and each changed row's value.

Your note on IOTracker: I agree that it deserves its own issue. This workflow does not open issues, so here is a draft for a maintainer:

Object-store I/O counts may miss scanner data reads. While measuring #1295, an object-store IOTracker around a dataset reported one 823-byte read for a delete that must read every uncovered fragment's id column. Lance's analyze_plan showed the real reads (8,000 rows, 124.6 KB, 12 I/O operations with 4 uncovered fragments). If the tracker misses scanner reads in general, the IoCounts cost curves and the DST storage-cost golden understate scan costs. Find out which reads the tracker sees, and either wrap the store the scanner uses or measure scans another way.

Local checks
  • Focused: merge_delete_selects_rows_by_rowid_issue_1295 (off, on, verify), delete_chunk_reads_by_filter_form_issue_1295, both chain_limit_tests chunk tests, stage_delete_does_not_advance_head_and_reads_through_staged: pass.
  • cargo dev-nextest -E 'package(omnigraph-engine) & (test(merge) | test(branch))': 309 passed.
  • GQT (cargo test -p omnigraph-gqt -p omnigraph-gqt-core): 462 corpus cases and the unit suites pass.
  • DST pinned suite (cargo test -p omnigraph-dst, from its crate directory): 56 passed, storage-cost golden unchanged.
  • cargo dev-clippy and cargo fmt --all --check: clean.

Base main 2d8effabe1900f63c30ecbf918c8fcfe3080f025. Debug build, gVisor host, RUST_MIN_STACK=16777216.

@ragnorc ragnorc 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.

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.

  1. stage_delete now keeps Lance's num_deleted_rows on the staged write. commit_staged_delete_chunks refuses 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.
  2. IdChunks<R> replaces the mixed type. DeleteIdChunks = IdChunks<u64> carries one _rowid per id. CandidateIdChunks = IdChunks<()> carries ids only. delete_expr exists 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.
  3. merge_delete_selects_rows_by_rowid_issue_1295 now runs off, on and verify. 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:

  • DeleteJob builds a scan with the chunk's filter. _rowid IN (...) becomes TakeOperation::RowIds. take_source turns 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 _rowid with no live row is not read. It is not an error.
  • num_deleted_rows is removed_row_addrs.len(), the rows the scan actually returned (lance-13.0.0/src/dataset/write/delete.rs:310-333). So a stale _rowid lowers the count, and the new check fails the merge.
  • Within one table lineage, a stale _rowid cannot land on a different live row. Lance assigns stable row ids from the monotonic next_row_id counter 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 the DeleteIdChunks::push comment 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

  1. 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."

  2. deleted_rows has two sources. In stage_delete it is Lance's count of the rows its scan returned. In stage_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 uses stage_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 after stage_delete_rows, it gets the requested count, not a checked one. Either say so on the field, or set it only in stage_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 ragnorc 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.

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.

  1. 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 _rowid can only miss its row.
  2. stage_delete_rows no longer sets deleted_rows (table_store.rs:3458-3463). The field is now Some only when Lance's delete scan counted the rows. The field doc and both accessor docs say this, and name stage_delete_rows as a None case.

How I checked it.

  • num_deleted_rows in stage_delete_rows is 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 calls stage_delete, never stage_delete_rows. The mutation path (exec/staging.rs:1004) calls stage_delete_rows, or stage_delete when 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 after stage_delete_rows now gets None (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 with enable_stable_row_ids: true (table_store.rs:3329).
  • The two unit assertions on deleted_rows() (staged_tests.rs:3235, :3279) both use stage_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.

@ragnorc

ragnorc commented Nov 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the delta review of a6334ad9. I took both optional items. New head: 711ef93b.

  1. The comment now says what the check proves. At the count check in commit_staged_delete_chunks, the comment now says the check compares counts, not row identity. It also says why a count is enough: stable row ids are never reused within a lineage, and only a target-side read supplies them. So a stale _rowid can only miss its row. It cannot hit another row.

  2. deleted_rows now has one source. Only stage_delete sets it, from Lance's own delete-scan count. stage_delete_rows deletes addresses that the caller supplies, so it now leaves the count unset (None). It no longer repeats the requested count. The field and accessor docs say this. The merge is the only reader, and it uses stage_delete, so nothing else changes.

Noted on the lance_surface_guards.rs text conflict with #1272 and #1235: keep both sides.

Local checks
  • Focused: merge_delete_selects_rows_by_rowid_issue_1295 (off, on, verify), delete_chunk_reads_by_filter_form_issue_1295, chain_limit_tests (5), stage_delete_does_not_advance_head_and_reads_through_staged, stage_delete_commit_rebases_over_disjoint_committed_delete: 9 passed.
  • cargo dev-nextest -E 'package(omnigraph-engine) & (test(delete) | test(merge))': 237 passed. This covers the mutation path that calls stage_delete_rows.
  • cargo dev-clippy and cargo fmt --all --check: clean.

Base main 2d8effabe1900f63c30ecbf918c8fcfe3080f025. Debug build, gVisor host, RUST_MIN_STACK=16777216.
Exact-commit CI at 711ef93b: 28 pass, 4 skipped, 0 fail.

@ragnorc
ragnorc added this pull request to the merge queue Nov 7, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Nov 7, 2026

@ragnorc ragnorc 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.

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.

  1. The merge takes main for 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.
  2. a04deb7f regenerates the snapshot. It puts this PR's line back (v0.15.0.md:152). It also adds the #1299 link to the nearest() line (v0.15.0.md:119), which main did not have yet.

How I checked it.

  • The PR's own diff is unchanged by the merge. git diff 2d8effab 711ef93b and git diff ea4a16d5 a04deb7f, both without the release file, differ only in four hunk line offsets in table_store.rs (+5 and +11 lines, from #1299's additions above them).
  • #1299 and this PR touch different code. #1299 changes nearest ranking (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 share table_store.rs but no function, and no caller of one reaches the other.
  • python3 scripts/release_notes.py verify --version v0.15.0 --target HEAD in 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 with main the 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 ragnorc 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.

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.

  1. 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).
  2. 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 a04deb7f with git diff 14753b56 5cf0c28e. Outside the release file, they differ only in hunk line offsets in table_store.rs, staged_tests.rs and lance_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 in engine/plan_source/indexes.rs and by TableStore::foldable_index_lag). This PR changes the merge delete path (exec/merge.rs, StagedWrite::deleted_rows, stage_delete). They share table_store.rs, but no function. The Lance surface guard delete_chunk_reads_by_filter_form_issue_1295 checks 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 --replace in the pinned docs environment. The only difference from the committed file is the header's target revision, 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 of crates/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 ragnorc 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.

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.

  1. lance_surface_guards.rs: the resolution keeps both guards. This PR's delete_chunk_reads_by_filter_form_issue_1295 comes first, then #1272's column_patch_keeps_blob_files_row_ids_and_key_index_coverage_issue_1264 (git show --remerge-diff 793e4e93). No line of either guard changed.
  2. docs/releases/v0.15.0.md: 8a1052f0 regenerates the snapshot. It keeps this PR's line and #1272's line, which now has its PR link, and moves the header's target to 793e4e93.
  3. 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 5cf0c28e and git diff e7e8e03b 8a1052f0 are identical (same hash of the +/- lines): exec/merge.rs, storage_layer.rs, table_store.rs, staged_tests.rs, merge_cost.rs, docs/dev/merge.md and the changelog entry. In lance_surface_guards.rs the 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 _rowid that 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 it MERGEABLE/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 of crates/omnigraph/tests/lance_surface_guards.rs, where each PR adds a guard. Keep both sides.
  • #1184 (50e3b863): conflict only in docs/releases/v0.15.0.md. Whichever merges second regenerates the snapshot.
  • #1304 (372c7ada): conflicts in docs/releases/v0.15.0.md and in a doc comment above copy_batch_range in table_store.rs. That second conflict comes from #1272, which is now on main, and #1304 has not merged main yet. 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.

  1. 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 include 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, the stage_delete tests, the engine's merge tests, merge_truth_table::merge_pair_truth_table, and #1272's update_of_one_scalar_patches_instead_of_rewriting_blobs_and_vectors_issue_1264, column_patch_keeps_blob_files_row_ids_and_key_index_coverage_issue_1264 and changes_cost::diff_of_a_column_patch_commit_takes_row_identity_issue_1264.
  2. 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].
  3. A temporary composition test (not for inclusion). Each row of a Doc type has a 64 KiB Blob and a Vector(8), and all 8 rows are in one fragment. feature deletes d3 and d5. main updates one scalar on d2, and the test asserts that this update staged UpdateMode::RewriteColumns. Then it merges feature into main. In each merge_lineage mode (off, on, verify), the merge's last Person transaction is a Delete whose predicate starts with _rowid IN (. 6 rows remain, d3 and d5 are gone, d2 keeps score = 1, and every remaining row keeps its vector. Result: pass. I removed the test afterwards. The worktree is clean.
  4. Release notes, in the pinned docs environment: python3 scripts/release_notes.py verify --version v0.15.0 --target HEAD gives Release notes OK: v0.15.0, 60bfd19d..8a1052f0. When I regenerated the snapshot with snapshot --target HEAD --date 2026-11-06 --replace, the only difference was the header's target revision. 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 ragnorc 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.

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.

  1. 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.
  2. 12c2bbeb regenerates the snapshot. Compared with 8a1052f0, the header gains #1304's changelog entry (link and digest) and moves target to ed35a1ed. 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 8a1052f0 and git diff 0b468487 12c2bbeb are identical once index lines 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.md and the changelog entry.
  • I checked the one new interaction, #1304 against this PR in table_store.rs. #1304 makes PendingScanAccount::retain the only batch charge of the pending-aware update scans (retain). Only scan_with_pending, scan_with_pending_materialized_blobs and scan_update_matches create that account. Their callers outside table_store.rs are exec/mutation.rs and exec/blob_write.rs. exec/merge.rs calls none of them. Its delete goes through stage_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 it MERGEABLE/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 of crates/omnigraph/tests/lance_surface_guards.rs, where each PR adds a guard. Keep both sides.
  • #1184 (c36bf618) and #1175 (draft, 4c93197b): conflict only in docs/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.

  1. 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 include 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, the stage_delete tests, merge_truth_table::merge_pair_truth_table and the _issue_1264 tests. #1304's regression is a GQT case, which this selection does not run (see the CI result below).
  2. cargo dev-nextest -E 'binary(lance_surface_guards) | binary(forbidden_apis)': 113 pass, 2 skipped by their own #[ignore].
  3. Release notes, in the pinned docs environment: python3 scripts/release_notes.py verify --version v0.15.0 --target HEAD gives Release notes OK: v0.15.0, 60bfd19d..12c2bbeb. When I regenerated the snapshot with snapshot --target HEAD --date 2026-11-06 --replace, the only difference was the header's target revision. 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.

@ragnorc ragnorc 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.

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.

  1. 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.
  2. 50bb8380 regenerates the snapshot. Compared with main at f0df4673, the header adds this PR's changelog digest and moves target to a22f0c84, and the body adds this PR's line. Compared with 12c2bbeb, the changes all come from main: 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 12c2bbeb and git diff f0df4673 50bb8380 are identical once index lines 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.md and the changelog entry.
  • #1307 has no interaction with this PR. It changes .github/workflows/build-release-archives.yml (notarization status read from notarytool info), docs/user/install.md, docs/releases/README.md and its own changelog entry. The only shared file is the release snapshot.
  • The head contains current main (f0df4673). GitHub reports it MERGEABLE.

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 in docs/releases/v0.15.0.md. It merges cleanly with main, so whichever of the two merges second regenerates the snapshot.
  • #1184 (c36bf618), #1175 (draft, 4c93197b) and #952 (00ad7927): conflict in docs/releases/v0.15.0.md, as they already do against main.
  • #1235 (draft, 1a856292) and #785 (0069e925): conflict at the end of crates/omnigraph/tests/lance_surface_guards.rs, as they already do against main. 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.

  1. 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 include merge_cost::merge_delete_selects_rows_by_rowid_issue_1295 and lance_surface_guards::delete_chunk_reads_by_filter_form_issue_1295.
  2. 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 HEAD gives Release 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's target revision. I restored the file. The worktree is clean.
  • check-docs.py OK (473 files); test_release_notes.py OK; check-release-note.py --self-test 21/21; git diff --check f0df4673 HEAD clean.

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.

Merged via the queue into ModernRelay:main with commit 2e7921c Nov 9, 2026
34 checks passed
ragnorc added a commit to ragnorc/omnigraph that referenced this pull request Nov 9, 2026
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.

performance: branch-merge deletes find their rows by the _rowid the merge walk already read, not by id IN (...)

1 participant