perf(changes): derive prunable commit intervals in O(delta) - #522
perf(changes): derive prunable commit intervals in O(delta)#522ragnorc wants to merge 14 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
d5cc430 to
29fcf18
Compare
|
Rebased onto the updated The base branch replaced the No behavior change to the pruning logic. Verified against the rebased base: engine builds, and |
8d870a4 to
5c87662
Compare
| } | ||
| // Pull the next id-ordered chunk of candidates, then probe the | ||
| // parent once for the whole chunk. | ||
| let mut chunk: Vec<RawRow> = Vec::new(); |
There was a problem hiding this comment.
[P1] Bound this queue by retained bytes and the current page budget, not only 8,192 rows. Before yielding one change, this holds all child RawRows, cloned IDs, the parent HashMap, and every queued Emit; each one-row Arrow slice can retain its full scanner batch. max_changes=1 still probes and compares all 8,192 candidates (including Blob payload tie-breaks), then truncation drops the remaining queue and the next page repeats the work. Large/multi-version eligible intervals can therefore exceed the page memory contract and amplify continuation I/O dramatically. Please add large-delta, page-size-1 byte/RSS and Blob-work coverage.
| /// string filter). The returned rows carry `_rowid`/`_rowaddr` and Blob | ||
| /// descriptions so `rows_equal` and `emitted_image` behave exactly as on the | ||
| /// full-merge path. | ||
| async fn probe_parent_images(parent: &Dataset, ids: &[String]) -> Result<HashMap<String, RawRow>> { |
There was a problem hiding this comment.
[P2] This is only BTREE-backed when the parent index fully covers every current fragment. Missing and stale derived indexes are normal; in that state Lance scans uncovered/all parent fragments once per chunk, so this path is still O(parent extent) and can become slower than the one-pass fallback. Gate the strategy on the existing TableStore::key_column_index_coverage(..., "id") == IndexCoverage::Indexed check (or use a one-pass alternative), and add absent/partial-index cost cells. The current flat test hides the cliff by calling ensure_indices() immediately before measurement.
| // interval's inserted/updated rows. Computed from the already-loaded | ||
| // manifests — no object-store data reads — so the candidate scan reads only | ||
| // O(delta) fragments. | ||
| let parent_ids: std::collections::HashSet<u64> = from_dataset |
There was a problem hiding this comment.
[P2] The implementation still walks and allocates over every parent fragment and then every child fragment, so CPU and retained metadata are O(dataset fragment count), not O(delta). The new cost test counts data-read operations only and cannot detect this term. Derive the surviving changed-fragment set by folding the already-loaded transaction operations, or narrow the advertised asymptotic and add a fragment-count/CPU-memory curve.
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.
5c87662 to
2014f1b
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
| self.ready.push_back(emit); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Probe chunk ignores page budget
High Severity
CandidateUpserts::next always materializes up to PARENT_PROBE_CHUNK (8,192) child RawRows, cloned ids, a parent image map, and a ready queue of Emits before yielding a single change. That retention is not bounded by the caller’s remaining row/byte page budget, so max_changes=1 still pays full-chunk probe/compare work (including Blob tie-breaks) and can exceed intended per-page memory on large eligible intervals.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2014f1b. Configure here.
…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.
aaltshuler
left a comment
aaltshuler
left a comment
There was a problem hiding this comment.
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.
| // walk; their stored decision is irrelevant. | ||
| let changed_fragments = | ||
| if scope.wants_kind(kind) && scope.wants_type_name(type_name) { | ||
| super::candidate_scan::interval_changed_fragments( |
There was a problem hiding this comment.
[P2] This correctness move places the history read under the final witness, but it still runs once per stateless page. Every continuation reconstructs plan_intervals and calls interval_changed_fragments, which re-lists/checks out the complete (begin, end] history (up to 1,024 versions) before returning even one change. With max_changes=1, a multi-version interval therefore pays the full history cost on every page, contrary to the advertised bounded O(delta) pagination. Please make this proof reusable through a safe immutable/pinned plan (not a forgeable trust bit in the checksum-only token), or narrow the claim and add a multi-version, page-size-1 transaction-read curve.
2 participants
What & why
The per-commit change enumerator derived one commit's entity changes by a full ordered-by-id merge of BOTH pinned table versions for every changed interval — O(table extent), not O(delta). A one-row update on a large table read the whole table on both sides. This was honestly pinned as a GROWING cost tripwire; RFC-030 §4.2/§4.3 specified the sanctioned fix (row-version candidate pruning + a transaction no-delete proof) and §14 recorded it as deferred. This PR ships it: the common insert/update/no-delete commit is now derived in O(delta).
The design
Per changed interval,
changes::candidate_scanproves whether the commit's effect is row-set-preserving — every Lance transaction in the interval isAppendor aRewriteRowsmergeUpdate(an exhaustive, wildcard-freeOperationmatch, so a new Lance variant compile-errors into review). When proven:Scanner::with_fragmentsto exactly the fragments the parent lacks (the child-minus-parent manifest diff — pure metadata, no data reads), with the_row_last_updated_at_version ∈ (begin, end]window dropping carried-over rows a fragment rewrite pulled along.id IN (chunk)index probe of the parent, reusing the same typedrows_equal/emitted_imagethe full merge uses (so it inherits the recent Blob descriptor-collision fix).Any unproven op (delete, overwrite, restore, compaction, a branch/lineage change, a non-advancing/oversized interval, a missing/cleaned transaction) falls back to the exact ordered merge, which classifies deletes correctly. The
EmitSourceseam yields the same id-orderedEmitstream as the oldnext_emit, so the streaming /ContinuationKey/ budgeting contract is unchanged.Correctness floor (guarded)
The classifier accepts every
Operation::Updateas non-deleting because OmniGraph'smerge_insertnever sets a by-source delete arm. Aforbidden_apis.rssource guard locks that (WhenNotMatchedBySource/when_not_matched_by_sourcemust be absent from engine source), so introducing one forces the classifier to be re-gated first.Backing RFC
Local verification
changes(38) andpoint_in_time(15) suites pass with pruning enabled — the candidate path produces the same results as the exact merge.changes_cost.rs::changes_page_opens_and_data_reads_are_bounded_by_deltaflipped fromassert_growstoassert_flat(data reads 10→9 across the extent sweep vs the old 11→23), with a companionchanges_page_unproven_op_scan_term_grows_with_table_extentkeeping the fallback honestly pinned as growing. The fixture reconciles theidBTREE (the production steady state, where the parent probe is an index lookup).commit_changes_falls_back_for_overwrite_that_removes_an_id— an overwrite that drops one id and changes another still surfaces the delete (pruning would miss it).forbidden_apis(18, updated.dataset()registrations for the new emitter),lance_surface_guards(32).cargo fmt --all --checkand both clippy graphs (-D warnings) clean; full failpoint-superset workspace test.Notes for reviewers
change-feed(feat(changes): commit entity diffs and a durable change feed #519) — this builds on the CDC enumerator + the Blob descriptor-collision fix. Retarget tomainonce feat(changes): commit entity diffs and a durable change feed #519 merges.idBTREE. On a never-optimized graph the parent probe full-scans (correct, slower); the feed's realistic steady state is periodically optimized.Note
High Risk
Touches change-feed derivation and keyed-write transaction provenance. A wrong prune can silently omit deletes; conservative fallbacks and ABA/failpoint coverage mitigate that.
Overview
Makes the common insert/update change-feed page O(delta) instead of a full parent/child ordered merge. Per interval,
candidate_scanwalks Lance transactions and prunes only when every op isAppendor aRewriteRowsUpdatewith a durable no-delete proof (omnigraph.no_by_source_deleteorinsert_absence). Proven intervals scan only child-minus-parent fragments in the row-version window and classify via batched parentid INprobes; anything unproven (delete, overwrite, missing history, incomplete version metadata, branch/lineage change) still uses the exact merge.Keyed
merge_insertnow stampsomnigraph.no_by_source_deleteat the single staging chokepoint so arepair --forceexternal merge cannot be pruned. Classification runs before the post-open head witness so a same-version branch recreate cannot retarget history. Cost tests pin the pruned path flat in table extent and keep the fallback growing; overwrite, marker, fragment-metadata, source-guard, and failpoint ABA cells cover silent-delete regressions.Reviewed by Cursor Bugbot for commit 4eaa4cf. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
The PR adds a guarded O(delta) path for deriving insert/update commit changes while retaining the exact ordered merge as the fallback.
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
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Changed table interval] --> B{Same identity and branch?} B -- No --> F[Exact ordered parent-child merge] B -- Yes --> C{All transactions proven row-set-preserving?} C -- No --> F C -- Yes --> D[Scan child-minus-parent fragments in version window] D --> E[Batch-probe parent by id] E --> G[Emit ordered inserts and updates] F --> H[Emit ordered inserts, updates, and deletes] G --> I[Shared pagination and byte budgeting] H --> IReviews (8): Last reviewed commit: "fix(changes): classify intervals under t..." | Re-trigger Greptile
Context used (3)