perf(changes): bound candidate scans to transaction footprints by ragnorc · Pull Request #522 · ModernRelay/omnigraph

ragnorc · 2026-08-16T18:19:42Z

Summary

prune only exact adjacent Lance versions whose pinned child transaction reads the published parent version
derive new child fragments and touched parent fragments from the transaction using binary search, without whole-manifest set walks
stream child candidates and parent before-images in id order with one prepared row per side
remove BTREE correctness and cost dependence, so absent and stale id indexes behave identically
size scans from the remaining page row/byte budget and stop on the sentinel before image, JSON, or Blob materialization
fall back conservatively for multi-version intervals, missing proof metadata, unsupported transactions, deletes, or any structural mismatch

Review fixes

This revision addresses the reported DX/performance cliffs:

no 8,192-row candidate queue on max_changes=1
no full candidate-batch VecDeque
no full parent/child fragment HashSet diff
no per-page transaction-history walk for non-adjacent intervals
no point-lookup cliff when the scalar index is absent or partially covered

New counters pin transaction reads, fragment metadata comparisons, candidate rows examined, scan targets, and image materialization.

Verification

cargo fmt --all --check
cargo clippy --workspace --all-targets --locked -- -D warnings -W clippy::dbg_macro
cargo clippy --workspace --all-targets --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints -- -D warnings -W clippy::dbg_macro
cargo test -p omnigraph-engine --test changes --test changes_cost --locked
cargo test -p omnigraph-engine --features failpoints --test failpoints change_feed_poll_classifies_intervals_before_the_head_witness --locked -- --exact
cargo test -p omnigraph-engine --test forbidden_apis --locked
cargo test --workspace --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints
scripts/check-agents-md.sh

Greptile Summary

The PR optimizes adjacent commit-change enumeration by deriving candidate scans from the published Lance transaction footprint while retaining the full ordered merge as a conservative fallback.

Adds transaction-provenance, fragment-shape, and row-version metadata gates for the pruned path.
Streams candidate and parent rows in ID order with page-sized scan targets and lazy row preparation.
Adds instrumentation, failpoint coverage, cost assertions, and documentation for the new planning path.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains in the eligible follow-up review scope.

No blocking failure remains.

Important Files Changed

Filename	Overview
crates/omnigraph/src/changes/candidate_scan.rs	Introduces the guarded transaction-footprint planner and streaming candidate emitter, with conservative fallback for unsupported or incomplete physical evidence.
crates/omnigraph/src/changes/enumerate.rs	Integrates pre-witness candidate planning and sentinel-aware page budgeting without an accepted new correctness finding.
crates/omnigraph/src/changes/row_compare.rs	Replaces eager per-batch row preparation with a lazy cursor and adds fragment-scoped scans with page-derived targets.
crates/omnigraph/src/table_store.rs	Adds durable transaction provenance used to distinguish engine-authored row-set-preserving updates from untrusted update shapes.
crates/omnigraph/tests/changes_cost.rs	Expands cost coverage for bounded transaction reads, fragment comparisons, candidate scans, and image materialization.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Changed table interval] --> B{Adjacent versions and same lineage?}
  B -- No --> F[Full ordered merge]
  B -- Yes --> C[Read pinned child transaction]
  C --> D{Proven row-set preserving and structurally valid?}
  D -- No --> F
  D -- Yes --> E[Build touched child and parent fragment plan]
  E --> G[Stream candidates and before-images by ID]
  F --> H[Emit exact logical changes]
  G --> H
  H --> I[Apply page budget and continuation]

_{Reviews (10): Last reviewed commit: "chore(ci): classify change-scan vocabula..." | Re-trigger Greptile}

Context used (3)

Knowledge Base — Branching and version history
Knowledge Base — Storage and table lifecycle
Knowledge Base — Data mutation pipeline

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a592fe228

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

ragnorc · 2026-08-17T13:39:09Z

Rebased onto the updated change-feed.

The base branch replaced the rows_equal scope hint (BlobComparisonScope) with an unconditional identity qualifier: managed-Blob descriptors are now keyed by the owning data file's immutable path (a globally-unique per-file UUID), which is authoritative across lineage without a same-branch/cross-branch distinction. The candidate-scan parent probe therefore calls the scope-free rows_equal directly, and the interim "adapt … scope" commit (which existed only to pass SameLineage to the old signature) is dropped as obsolete. build_change_feed_cut is already registered in the surface guard on the base.

No behavior change to the pruning logic. Verified against the rebased base: engine builds, and changes (42), changes_cost (6, including the pruned-FLAT / fallback-GROWING gates), and forbidden_apis (18) all pass.

cursor

Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 2014f1b. Configure here.}

aaltshuler

Follow-up review of 4eaa4cf: the row-version metadata and post-witness branch-ABA P1s are correctly fixed and their threads are resolved. The PR is still not merge-ready: the 8,192-candidate queue remains unbounded by the page row/byte budget; full-index coverage is still assumed; fragment discovery remains O(total fragments); and the inline comment below covers repeated transaction-history work on continuation. CI is currently red on two trivial needless_borrow Clippy errors. The PR also still targets the already-merged change-feed branch; it must be rebuilt/retargeted onto current main and revalidated there.

First stage of the candidate-pruning optimization. changes::candidate_scan classifies whether a changed table interval can be derived in O(delta): every transaction in (begin, end] must be Append or a RewriteRows merge Update (row-set-preserving), with same branch/identity, an advancing bounded version interval, pinned handles, and active stable row IDs. Any doubt returns Ok(false) so the caller falls back to the exact ordered merge; it never errors on a normal miss (cleaned history). The Operation match is exhaustive with no wildcard, so a new Lance variant is a compile error that forces review (§9). Not yet wired into the enumerator (#[allow(dead_code)] until the wiring stage). Unit-tested over synthesized operations.

The CDC candidate-pruning classifier treats every persisted Operation::Update as row-set-preserving, so it can derive the change feed without a delete pass. That is sound only because OmniGraph's merge_insert never deletes an unmatched-by-source row. Lock that floor with a source-walk guard in forbidden_apis.rs: a by-source merge arm (WhenNotMatchedBySource / when_not_matched_by_source) must be absent from engine source, so introducing one forces the classifier to be re-gated first. The invariant holds today (neither symbol appears in src).

…C-030 §4.2) Make the per-commit change enumerator O(delta), not O(table), for the common insert/update/no-delete commit. Per changed interval, changes::candidate_scan proves whether the commit's effect is row-set-preserving (every transaction in the interval is Append or a RewriteRows merge Update); if so it derives the changes by scanning only the commit's changed fragments (the child fragments absent from the parent, from the manifest diff) plus a batched exact-id BTREE probe of the parent for before-images, classifying via the same typed rows_equal / emitted_image the full merge uses. Any unproven op (delete, overwrite, restore, compaction, unknown) falls back to the exact ordered merge. A prunable interval has zero logical deletes (one transaction per commit + the D2 rule + no delete-capable merge arm), so the pruned path needs no delete pass. The EmitSource seam yields the same id-ordered Emit stream as next_emit, so the streaming / ContinuationKey / budgeting contract is unchanged — the full changes and point_in_time suites pass identically with pruning enabled. Flip the changes_cost tripwire from assert_grows to assert_flat (data_reads 10 -> 9 across the extent sweep vs the old 11 -> 23) and add a fallback tripwire that keeps the unproven-op path honestly pinned as growing. The cost fixture now reconciles the id BTREE so the parent probe is an index lookup (the production steady state).

…rite RFC-030 §9 L0 guard: an overwrite that drops one logical id and changes another must still surface the delete. The classifier rejects Operation::Overwrite, so the enumerator falls back to the exact ordered merge and reports both the delete of the dropped id and the update — a candidate scan of the child's new fragments alone would never see the dropped id and would silently lose the delete. Passes with the optimization enabled, guarding against a future mis-prune of a row-removing op.

Move the row-version candidate pruning + no-delete proof from deferred to shipped in RFC-030 §14, and update the changes_cost.rs testing-map row to describe the flat pruned tripwire + the growing fallback tripwire.

…ites OmniGraph's keyed merge_insert never uses a delete-capable by-source arm, so every keyed-write Operation::Update removes no unmatched rows. Stamp a durable omnigraph.no_by_source_delete transaction property at the single general keyed merge chokepoint (staged_keyed_merge_result) so a downstream reader can trust a *persisted* Update was delete-free — the op shape plus the source-walk guard prove only that current engine code builds no such arm, not that a persisted transaction (e.g. one adopted from an external merge via repair --force) is. The marker is read-advisory: stamped unconditionally, it survives commit and recovery (it lives in Lance's committed manifest; recovery reuses the landed version), and a missing marker only costs an optimization. It is distinct from the RFC-023 insert_absence certificate (minted only for pure inserts), so a real update-bearing upsert carries the marker but not the certificate.

… Update The candidate-pruning classifier trusted any Operation::Update{RewriteRows} as row-set-preserving, but its child-only fragment scan has no delete pass. An external Lance merge with a delete-capable by-source arm — adopted as uncovered drift via repair --force --confirm — persists that exact shape, and its removed rows would be silently dropped from the diff and feed. Gate Update pruning on a durable OmniGraph provenance proof: transaction_is_row_set_preserving requires the no_by_source_delete marker or the insert_absence certificate for a RewriteRows Update; Append stays unconditional. A marker-less external Update falls back to the exact ordered merge, which reports the deletes. The op-shape classifier is retained (its exhaustive match still fails a new Lance variant into review) and the forbidden_apis source guard stays as defense-in-depth. The new unit test pins that a marker-less Update{RewriteRows} is not row-set-preserving while a marked/certified one is; the existing upsert-prune cost and image tests stay green as live end-to-end proof that the write path stamps the marker and the classifier honors it.

Rewrite RFC-030 §14's candidate-pruning justification — a RewriteRows Update is trusted delete-free only with a durable per-transaction provenance marker, not the source-walk guard alone; flip the deferred per-write certificate to shipped. Touch §4.3 and §9 L0 to require the marker, and note the write-path stamp cell and classifier gate test in testing.md.

The classifier checked only the dataset-level uses_stable_row_ids() flag, but the pruned path's correctness rests on each CHANGED FRAGMENT's _row_last_updated_at_version sequence: pinned Lance 10 silently fills the column with 1 when a fragment's sequence is missing or fails to load (the stream reader's 'Default to version 1 if sequence not provided' arm, which also swallows a failed load_sequence()). For any interval with begin > 1 those rows fall outside the candidate window and real updates vanish without an error. Before returning the changed set, require every changed fragment to carry present, decodable last-updated metadata (a manifest-level check, no data reads); any gap is a normal miss that falls back to the exact ordered merge, which never consumes the version column. Unit test pins the missing-metadata fragment as not loadable; the pruned cost/image tests staying flat/green are the live positive proof that real keyed-write fragments pass the gate.

RFC-030 §11 claimed the C0-C4 core persists nothing, which the no_by_source_delete transaction property made stale. Record the marker in the format audit with the explicit no-format-bump conclusion: it is read-advisory in every direction (missing marker = exact-merge fallback, older binaries ignore unknown properties, recovery and publication never consult it), an optimization-eligibility proof rather than a stored watermark or tombstone. Precision fixes: the marker is stamped by every GENERAL keyed MergeInsert update — proven strict inserts carry insert_absence instead — corrected in §14, the module docs, and the constant's doc comment. §14 also records the new per-fragment row-version-metadata loadability gate. Status honesty: the header no longer reads as unqualified shipped — it names the two recorded open obligations gating full acceptance (the §4.4 ordered-scan memory bound and bounded client auto-pagination), and the C2 phasing row annotates the aggregating helpers as open. §3.3 records name-only type filtering as a deliberate v1 scope decision with the sanctioned ID-filter extension path.

…n sequences Two red regressions against the fragment loadability gate: - a fragment whose last-updated sequence is stored in an external file currently PANICS the poll: pinned Lance 10's load_sequence() External arm is todo!(), and the gate probes it instead of classifying it - a sequence that decodes cleanly but covers fewer rows than the fragment holds currently passes the gate; Lance's single-run fast path would then stamp that run's version across every requested row Both must classify as not-loadable so the interval falls back to the exact ordered merge.

…t coverage The loadability gate now matches the Inline variant explicitly and decodes its bytes directly, making pinned Lance 10's panicking External load_sequence() arm structurally unreachable, and requires the decoded sequence to cover exactly physical_rows — a decodable-but-short sequence would otherwise let the single-run fast path stamp one version across every requested row. Absent, external, undecodable, short, or row-count-less fragments all fall back to the exact ordered merge.

New failpoint cell: park a poll after the final post-open logical head witness, delete/recreate the polled branch at the same table versions with provably row-set-preserving replacement history, and resume. The interval classifier currently walks (begin, end] transactions live at that point — version manifests sit at replaceable numeric paths, unlike UUID-named data and transaction files — so it classifies the original delete commit from the replacement's marker-carrying history, prunes, and returns a successful page whose block silently omits the delete. Red: the page for the delete commit carries zero changes. A poll may fail loudly across an in-flight branch recreation, but a page it does return must carry the original commit's changes.

Move the candidate-pruning decision from EmitSource::plan into plan_intervals, before the two reprove_named_branch_heads calls. The classifier's (begin, end] transaction walk is a live read of numeric-path version manifests — the one replaceable read on the pruned path — so it must be covered by the same witness that covers the table opens. EmitSource::plan now consumes the precomputed decision and performs no history read; scope-filtered intervals skip the walk entirely. The CHANGE_FEED_POST_HEAD_WITNESS failpoint cell turns green: a delete/recreate after the witness can no longer reroute an interval to replacement history.

chatgpt-codex-connector Bot reviewed Aug 16, 2026

View reviewed changes

Comment thread crates/omnigraph/src/changes/candidate_scan.rs

ragnorc force-pushed the cdc-candidate-pruning branch 2 times, most recently from d5cc430 to 29fcf18 Compare August 17, 2026 13:38

ragnorc force-pushed the cdc-candidate-pruning branch 4 times, most recently from 8d870a4 to 5c87662 Compare August 18, 2026 12:07