Skip to content

fix(changes): take the exact merge for a window across a merge commit - #1220

Open
ragnorc wants to merge 1 commit into
ModernRelay:mainfrom
ragnorc:claude/priceless-heisenberg-29a470
Open

ragnorc wants to merge 1 commit into
ModernRelay:mainfrom
ragnorc:claude/priceless-heisenberg-29a470

Conversation

@ragnorc

@ragnorc ragnorc commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Change discovery by row identity (#1201) pairs two table versions' live rows by _rowid and skips pairs whose _row_last_updated_at_version stamps are equal. That is sound only when the from table version is on the to version's Lance read_version chain. The guard checked graph first-parent ancestry instead, and a merge commit separates the two: a merge can switch a table's pin to the merged branch's pin (classify_adopt's pointer switch, which a fast forward does for every type the merged branch changed), and that pin's history never read the target's pin when the merged branch had merged the target since the fork.

Reproduced before the fix:

  1. From one base, main inserts x and feature inserts y. Both are first commits after the fork, so Lance gives both rows one _rowid and one stamp.
  2. Feature merges main, which re-inserts x under a new _rowid.
  3. Main merges feature as a fast forward.

On the unfixed tree, both new GQT cases fail under verify (the debug default) with row-identity change discovery verify divergence for 'node:Person': exact merge yields insert 'y', row identity yields insert 'x'. Under on (the release default) the row-identity answer is the one returned, so the two-point diff from main's head before the fast forward reports insert x and silently drops y. The commit page and the change feed take the same row-identity route for the fast-forward commit (ChangeDiscovery::linear set from_is_ancestor: true unconditionally). The branch form had the same gap: when a fast forward sits above a merge base that is on the source's first-parent chain, branch diff feature into main gave the same wrong answer.

Fix

Row identity now requires that to descend from from through first parents with no merge commit after from, in all three forms:

  • commit page and feed: ChangeDiscovery::for_commit turns pairing off when the commit has a merged parent;
  • two-point diff: diff_descends_linearly (formerly diff_from_is_ancestor);
  • branch review: source_descends_linearly_from_base (formerly base_on_source_chain).

The commit-graph walk (CommitGraph::descends_linearly_from, formerly first_parent_chain_holds) stops at the first merge commit above from. A held from with a merge commit above it no longer answers early, so the coordinator's holds_commit shortcut is gone; the walk still reads held commits from memory.

The adjacency path is unchanged: a type that an interval reaches through one recorded transaction still takes it, whatever commit wrote it.

Cost

A merge commit's page and a window that crosses a merge commit now take the exact ordered merge for every type that has no single-transaction proof. That is what they took before #1201, so nothing regresses against 0.14.1. It does give back part of #1201's gain: three-way merge pages (a chain of insert, update and delete chunks) and clean fast forwards, where pairing was safe. Proving each type's own lineage (its pin's read_version chain back to from's pin) would bring that back. That proof is left to a follow-up, recorded in the RFC. The graph_diff_of_diverged_heads_takes_the_exact_merge cost owner now records the route a window across a fast forward takes.

Tests

  • cases/graph_diff_across_a_fast_forward.gqt: the two-point diff across the fast forward under the default, on and off settings.
  • cases/branch_review_across_a_fast_forward_on_the_source.gqt: the branch-form variant.
  • integration/changes.rs::commit_changes_feed_and_diff_across_a_fast_forward_report_the_source_rows: the commit page, feed and two-point diff under off, on and verify.
  • integration/changes_cost.rs::graph_diff_of_diverged_heads_takes_the_exact_merge: route counters for a window across a fast forward.

Docs: docs/dev/lance.md (Row version stamps), docs/user/branching/changes.md, the RFC's ancestry restriction and decision log. The unreleased graph-diff-row-identity.changed.md note is narrowed to windows with no merge.

Row-identity change discovery pairs two table versions' live rows by
`_rowid` and skips pairs with equal version stamps, which proves an
unchanged row only when the `from` version is on the `to` version's
read-version chain. The guard checked graph first-parent ancestry
instead. A merge can switch a table's pin to the merged branch's pin,
and a fast forward does so for every type the merged branch changed;
when that branch had merged the target since the fork, its history
never read the target's pin. Sibling first inserts share a row id and a
stamp, so the fast forward's commit page, the feed across it, the
two-point diff from its parent and a branch review with a fast forward
above the base reported the target's own row as inserted and dropped
the merged branch's (refused under `verify`).

Pairing now requires that `to` descend from `from` through first
parents with no merge commit after `from`: the commit page and the feed
skip it for a merge commit, and the commit-graph walk behind the two
diff forms stops at the first merge commit above `from`. A type with a
one-transaction adjacency proof keeps the candidate path; a merge
commit's other types take the exact merge, as before row identity.

ragnorc commented Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

Why

Row-identity change discovery pairs live rows by _rowid. It skips a pair when the version stamps are equal. In the pinned Lance 13.0.0, Transaction::build_manifest_prepared sets that stamp from the read manifest version plus one (lance-table manifest_build.rs). do_commit_detached_transaction then stores a random manifest version (lance src/io/commit.rs). Two first commits that read the same parent therefore give the new row one row id and one stamp. Equal stamps prove an unchanged row only on one read_version chain.

A merge can install the other branch's table pin. classify_adopt adopts the source version. A fast forward still publishes one graph commit with merged_parent, and it does this for every type the merged branch changed. The old guard treated graph first-parent ancestry as the table chain. The diff, the commit page, the change feed, and a branch review could then report the target's row as inserted and drop the merged branch's row.

What this change does

Pairing now requires linear first-parent descent, with no merge commit after from. The commit page and the feed skip pairing for a merge commit. Both diff forms use the same walk. The coordinator holds_commit shortcut is gone. A type with a one-transaction adjacency proof still uses the candidate path.

Recommendation

Revise. The first-page rule is the right repair for the reported bug. One later page of a branch diff still applies row identity to a frozen window that crosses a merge. The release-note check also fails on this commit.

Blocking

  1. Branch-diff continuation recomputes the flag from the live merge base. The inline comment has the failing probe. Decide the flag from the token's from and to.
  2. The PR Title check failed on this commit (run 41260338267). scripts/check-release-note.py counts only added changelog.d files (git diff --diff-filter=A). This PR edits changelog.d/graph-diff-row-identity.changed.md. That edit correctly narrows the unreleased note for performance: graph diff and branch review read both full tables unless the endpoints are one transaction apart #969. It does not satisfy the check. Add a new changelog.d/<slug>.fixed.md for this bugfix.

Optional

The sound condition is each opened table pin's read_version chain, not a further graph-shape rule. The graph rule is a sufficient proxy for one page when the walk starts at the commit that page opens. It is also broader than the pin chain. A clean fast forward whose pins still descend from the target loses pairing. graph_diff_of_diverged_heads_takes_the_exact_merge records that route. Drop and re-add already return the exact merge, because the schema identity does not match. A linear overwrite does not reuse old row ids, because next_row_id is kept. CreateIndex, compaction, and a schema project stay on the chain. Please do not add more graph-shape exceptions. One function should answer the question for the two commits a page opens. The first page and the continuation should both call it. The RFC follow-up, a per-pin read_version proof, should replace the proxy.

Tradeoff and liability

Removing holds_commit does not add history reads on a long branch. The hot buffer is still the first-parent run that ends at the head (HISTORY_RELEASE_BYTES, 256 KiB). Commits outside it already come from __history. The walk reads at most 1024 commits per step (FIRST_PARENT_WALK_COMMITS). The cost that remains is an exact-merge scan for every window that contains a merge. That cost is the right price until a pin-chain proof exists.

This change removes the silent mis-report on every door that decides the flag once, for the commits it opens. It adds a second decision on the continuation, taken from the live heads. That second decision is the remaining liability. Five more graph-shape exceptions would leave the cause in place.

PR 1184 also edits commit_graph.rs and graph_coordinator.rs. It changes which branch serves a held history block. It does not implement this descent rule. The two diffs need a careful combine.

Validation

Head 028127f11b551db4878a9d5a1907cf0d0d06ed9e. Merge base f728f01e1543c442eff77f8b0d10c223e38da36d.

Local, head, pass:

cargo test -p omnigraph-gqt --locked --test gq_logic_tests -- across_a_fast_forward

Result: ok. 2 passed. graph_diff_across_a_fast_forward.gqt and branch_review_across_a_fast_forward_on_the_source.gqt.

Local, merge base, with those two case files copied in and not committed, fail for the predicted reason:

same command. Result: 0 passed; 2 failed. Both cases stop at branch diff with row-identity change discovery verify divergence for 'node:Person': exact merge yields insert 'y', row identity yields insert 'x'.

Local, head, temporary test in integration/changes.rs, then removed and not pushed. Feature inserts y and then a. Main inserts x. Feature merges main. A branch review with one change per page and change_lineage = on returns insert a. Main then fast-forwards. The live merge base is the source head. The next page returns insert x, with change_intervals_row_identity = 1 and change_intervals_exact_merge = 0. The exact merge of the frozen commits is insert a and insert y.

CI on this commit: Test Workspace, GQT (ordinary), GQT (dst), and GQT (served) passed. PR Title failed as above. Devin Review skipped.

Limitations

GQT cannot express a page token. The continuation probe is not in the PR. Please add it under integration/changes.rs. I did not re-run the new commit-page, feed, and cost tests locally. I did not time exact-merge scans on a long history. The two-point continuation was read and not executed. A new merge on the live to head stops that walk, so ordinary forward motion takes the exact merge and returns the same rows. The demonstrated wrong rows are the branch form.

Inline comment on crates/omnigraph/src/db/omnigraph.rs line 3296 (RIGHT):

reopen_diff_continuation sets linear_descent from the live merge base (source_descends_linearly_from_base). It then opens the token's from and to.

Those commits differ once the target moves. After main fast-forwards to feature, the live base is the source head, so this call returns true. The next page pairs rows on the frozen window. That window still crosses the merge.

A temporary test on this commit showed the wrong row. Page 1 used change_lineage = on and one change per page. It reported insert a by the exact merge. After the fast forward, page 2 reported insert x by row identity (change_intervals_row_identity = 1, change_intervals_exact_merge = 0). The exact merge of those frozen commits is insert a and insert y.

docs/user/branching/changes.md says a page token keeps the commits the first page resolved. Later pages answer for those commits after the branches move. DiffPageTokenV1 says the same. The token does not store this flag. Do not trust a client bit. Walk first parents from the token's to, and stop at from's generation. read_first_parents already takes a start id. descends_linearly_from always starts at the live head.

The two-point arm above walks the live to head in the same way. Ordinary forward motion meets a new merge and takes the exact merge. That returns the same rows at a higher cost. Please make both arms use the commits the page opens.

@ragnorc ragnorc added the skip-changelog Typed change with nothing users can see; waives the changelog.d note check label Nov 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog Typed change with nothing users can see; waives the changelog.d note check

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant