Fix history independence with Dolt-style streaming chunker - #185
Merged
Conversation
ProllyTree mutations (insert/insert_batch/delete/delete_batch) now call
canonicalize(), which rebuilds the tree from its leaf-level (key, value)
pairs so the resulting shape and root hash depend only on the final set
and the TreeConfig, never on operation history. The rebuild chunks the
sorted sequence with the existing rolling-hash chunker, emits leaves,
then iteratively chunks each level's pivots until a single root remains.
Add a history-independence test suite at three layers:
* ProllyNode unit tests in src/node.rs - the primitive layer that
bypasses canonicalize still has order-dependent balance behavior;
these tests are #[ignore]d as the acceptance suite for a future
incremental rebalance.
* ProllyTree unit tests in src/tree.rs - 4 always-on tests covering
root hash equality across orders, plus update and delete paths.
* tests/history_independence.rs - 6-test integration matrix covering
(config x N x key-pattern x order x op-mix) at the public API.
Trade-off: canonicalize is O(N) per mutation, so an N-key insert is
O(N^2) overall. Acceptable as a correctness-first stop-gap; the
incremental fast path can return once the node-level tests validate it.
The canonicalize fix in ProllyTree::insert/delete is inherited by the git-backed store layers because both ultimately call through to the ProllyTree mutation API. Add tests at those layers to verify: - GitVersionedKvStore: insert/update/delete sequences in different orders produce identical committed tree root hashes (3 tests in src/git/versioned_store/tests.rs). - GitNamespacedKvStore: per-namespace root hashes are order- independent, including under cross-namespace interleaved writes with multiple intermediate commits (3 tests in src/git/versioned_store/namespaced_tests.rs). All 6 new tests pass under the default, git-only, and minimal feature flag combinations.
The merge path was already using ProllyTree::insert / delete to apply
adds, modifications, and removes to a destination tree, so it inherits
the canonicalize step for free. Add three tests that pin this:
* apply_merge_results_is_canonical - merging two divergent trees
produces a result whose root hash matches a fresh canonical build
of the same final key/value set.
* apply_merge_results_independent_of_result_order - shuffling the
merge results before apply produces the same root hash.
* merge_trees_ignore_conflicts_is_canonical - the higher-level
convenience wrapper also produces a canonical result.
All three pass without further code changes.
VersionedKvStore::commit and NamespacedKvStore::commit_impl both drained their staging area by calling ProllyTree::insert / delete for each item - and since each ProllyTree mutation triggers a full canonicalize (O(tree size)), a commit with K items cost O(K * tree_size). Add ProllyTree::apply_changes(impl IntoIterator<Item = (Vec<u8>, Option<Vec<u8>>)>) which materializes the current state into a BTreeMap, applies all changes, then rebuilds the tree canonically once. Wire the two commit paths through it; namespaced commit collects the externalize threshold transformation into the batch before applying. Result: commit cost drops from O(K * N) to O(N + K log K), a meaningful win for any commit that stages more than a handful of changes.
Attempted a focused fix to lift the canonicalize stop-gap (a `removed` flag on ProllyNode set when a delete empties a node, with parent dropping it). The change compiled but broke node::tests::test_delete because the existing `merged` / `split` two-bit signaling protocol isn't expressive enough to share state with the new flag. Reverted. Documented why a small fix isn't tractable and what the correct redesign looks like: a richer BalanceOutcome signal carrying "remove range [l..r] and insert these new children", with the rebalance loading both left and right neighbors and recursing up to the root. The canonicalize stop-gap remains in place. The #[ignore]d node-level tests in src/node.rs are the acceptance suite for the redesign work.
Add src/streaming_chunker.rs - a Rust port of Dolt's streaming chunker pipeline (chunker.go + node_splitter.go + node_builder.go). The chunker streams sorted (key, value) pairs through a Splitter that resets at every chunk boundary; each chunk's split decision depends only on the items inside that chunk, so the same sorted input always produces the same root hash by construction. Phase 1 scope: no cursor support yet. The chunker is wired in as the backend for ProllyTree::canonicalize and apply_changes, replacing ProllyNode::build_canonical_from_pairs. Per-mutation cost is still O(N) until Phase 2 adds NodeCursor + advance_to for fast-forward over unchanged subtrees. Tests: 5 new unit tests in src/streaming_chunker.rs verify empty inputs, single-item, sub-min sequences, order-independence, and parity with build_canonical_from_pairs at N=1024. Existing suite of 154 lib tests + integration matrix all pass. Perf: cargo bench --bench tree -- insert_single_10000 drops from 30.1s (canonicalize stop-gap) to 13.3s - 2.3x speedup. Still 11.7x slower than the pre-fix baseline (1.14s), which is what Phase 2 closes.
Add NodeCursor (Rust port of Dolt's tree.cursor): a stateful linked
list of cursors, one per tree level, with `at_key`, `at_start`,
`advance`, etc. Add `apply_mutations(root, sorted_mutations, config,
storage)` which walks the old tree with the cursor and streams
unchanged + mutated items through a fresh chunker, producing a
canonical new tree.
ProllyTree::{insert, insert_batch, delete, delete_batch} now route
through `apply_changes -> apply_mutations`. The old in-place
`self.root.insert/delete` calls (which couldn't maintain history
independence) are gone from the public mutation path.
Tests: 5 new apply_mutations cases (empty tree, no-op pass-through,
insert middle, deletes, updates) all pass; 159 lib tests + integration
matrix + all other suites green.
Perf: bench at N=10k went from 30.1s (canonicalize stop-gap) -> 13.3s
(Phase 1 streaming) -> 16.6s (Phase 2 cursor). The Phase 2 number is
slightly worse than Phase 1 because the cursor adds overhead without
yet enabling the fast-forward skip. Adding the cursor fast-forward
(matching Dolt's chunker.advanceTo) is the next step and is what
brings the per-op cost back to O(log N + edits).
Refresh docs/bench-canonicalize-impact.md and
docs/dolt-streaming-chunker.md with the bench numbers and
status after Phase 1 (streaming chunker) and Phase 2 (cursor-driven
apply_mutations).
State after these phases:
* Public API at every consumed layer (ProllyTree, GitVersionedKvStore,
GitNamespacedKvStore) has history-independence by construction -
no more post-hoc canonicalize rebuild.
* Bench at N=10k: 16.6s (14.5x slower than pre-fix baseline of 1.14s)
* The cursor fast-forward (chunker.advanceTo) is the next step and
is what closes the perf gap to ~2x of baseline.
…orward) Add two complementary fast paths to apply_mutations that recover most of the per-op cost lost in Phase 2 (cursor-driven walk): 1. **Pure-append** (`try_pure_append`): triggered when every mutation is an insert past the tree's max key (common in append-only and monotonic-key workloads). Walks the tree's leaves once to collect (firstKey, leaf_hash), feeds all but the last leaf directly to the level-1 chunker via `Chunker::append_subtree_at_parent_level`, then re-processes only the last leaf's items + new keys through level-0. Skips reading the items of N-1 leaves. 2. **Alignment-aware fast-forward** (`fast_forward_to_end`): after each chunker boundary emit, the chunker exposes its `last_emit_hash`. When the cursor was at the end of an old leaf and the emitted hash matches that leaf's hash, the chunker has re-synced with the old tree. If no more mutations are pending, we copy the remaining old leaves directly to the level-1 chunker without reading their items. Both paths preserve canonicality: they feed the same sequence of (firstKey, hash) entries to the level-1 chunker that a full canonical rebuild would, just without re-walking unchanged leaf items. Tests: the 6 history-independence tests at the ProllyTree API layer + the integration matrix + the 10 streaming-chunker unit tests all pass. Bench: insert_single at N=10k drops from 16.6s (Phase 2) to 11.2s (Phase 3), a 32% speedup. vs the canonicalize stop-gap (30.1s) the total speedup is 63%. The remaining gap to the pre-fix baseline (1.14s) is the inherent O(N²) of one-mutation-per-call and the cost of writing new tree nodes to storage on every commit.
The canonicalize method was removed in 2c019bf when the streaming chunker took over. Update all the comment/doc references that still mentioned it across the codebase: src/streaming_chunker.rs, src/node.rs test-module header, src/tree.rs merge tests, src/git/versioned_store/{tests,namespaced_tests,namespaced,core}.rs, and tests/history_independence.rs. Also annotate ProllyNode::build_canonical_from_pairs as the alternate canonical builder kept as an independent oracle in the matches_node_build_canonical_from_pairs unit test - it is no longer in the production path, the streaming chunker handles every mutation.
…oadmap
Two cleanups now that the streaming-chunker port is the only mutation
path and the public-API + integration tests cover history independence
end-to-end:
1. Remove the 6 #[ignore]d tests in src/node.rs and their dedicated
`build_node` helper. These exercised the legacy in-place
`Balanced::balance` path and documented bugs (chunk-boundary drift
under non-default configs, empty trailing leaves after delete,
internal-node structure divergence) that no longer matter for the
public API. Coverage of history independence now lives in:
* src/streaming_chunker.rs (10 unit tests)
* src/tree.rs::history_independence_tests (4 tests)
* src/tree.rs::merge_canonicality_tests (3 tests)
* src/git/versioned_store/tests.rs::history_independence_tests (3)
* src/git/versioned_store/namespaced_tests.rs (3 tests)
* tests/history_independence.rs (6 integration tests)
The narrow `test_history_independence` and the lifted-to-u64
`test_history_independence_default_config_traversal` remain as
ProllyNode-level smoke checks against the legacy path. Updated the
module header to reflect the new state.
2. Remove docs/incremental-rebalance-roadmap.md. It described the
earlier "rewrite the in-place balance" plan that was abandoned in
favor of the Dolt streaming-chunker port. Fully superseded by
docs/dolt-streaming-chunker.md, which now documents Phase 4 as done
and lists the remaining optional follow-ups (recursive fast-forward,
multi-mutation batch advance, legacy balance removal).
Suite: 159 lib tests pass, 0 ignored, 0 failed.
zhangfengcdt
changed the title
…<2 entries The Python CI hit a segfault (release) / u8 overflow panic (debug) in basic_usage.py at the file-backed `insert` step. Root cause: Python's `TreeConfig.__init__` has `pattern=0` as its default, and the example also sets `min_chunk_size=1`. With pattern=0 the splitter's `(hash & pattern) == pattern` check is true on every item, so a boundary fires on every append. The chunker's `handle_boundary` then recursively created a new parent chunker one level up, whose splitter saw the just-emitted `(firstKey, hash)` entry, fired a boundary, and cascaded - climbing the `u8` level counter until it overflowed. This is Dolt's "constraint (3): internal nodes must contain at least 2 key-value pairs" rule from `chunker.go::append`, which we didn't port. Adding it: at level > 0, defer boundary emission until the in-progress chunk has at least 2 entries. Leaf-level chunks (level 0) are still allowed to be single-entry so we can store a 1-key dataset. The old in-place `Balanced::balance` didn't have this bug because it emitted upward only when the chunker reported >1 chunks (its `chunks.len() <= 1` early-return). The streaming chunker emits at every boundary, so we need the explicit constraint here. Regression test in `streaming_chunker::tests` reproduces the degenerate config and asserts the level stays bounded. Verified by re-running basic_usage.py against a debug + release build, both succeed end-to-end. Rust suite: 160 lib tests pass, 0 failed.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a Rust “streaming chunker” mutation pipeline intended to restore ProllyTree history independence (same final (key, value) set ⇒ same root hash regardless of operation order) and adds a comprehensive test matrix across API layers to prevent regressions.
Changes:
- Adds
src/streaming_chunker.rsimplementing a cursor-driven streaming mutation engine (Splitter/Chunker/NodeCursor+apply_mutations) plus fast paths. - Routes
ProllyTreemutations through a newapply_changesbatch API and updates git-backed KV store commits to apply staged changes in a single batch. - Adds integration/unit tests and documentation explaining the mutation pipeline and history-independence guarantees.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/streaming_chunker.rs |
New streaming chunker + cursor-based mutation application and performance fast paths. |
src/tree.rs |
Routes public mutation APIs through apply_changes and adds history-independence tests. |
src/node.rs |
Adds a canonical “oracle” builder (build_canonical_from_pairs) and updates/extends tests/comments. |
src/git/versioned_store/core.rs |
Applies staging in a single batch via apply_changes during commit. |
src/git/versioned_store/namespaced.rs |
Drains namespace staging into a batch and applies via apply_changes once per namespace. |
src/git/versioned_store/tests.rs |
Adds history-independence tests at the GitVersionedKvStore layer. |
src/git/versioned_store/namespaced_tests.rs |
Adds history-independence tests for namespaced stores and interleaving. |
tests/history_independence.rs |
New integration test matrix covering configs × orders × key patterns × op mixes. |
src/lib.rs |
Exposes the new streaming_chunker module. |
docs/theory/mutation.md |
New documentation page describing the streaming mutation pipeline and fast paths. |
docs/theory/index.md |
Adds the new mutation pipeline doc to the theory index. |
mkdocs.yml |
Adds the new documentation page to the site nav. |
src/proximity/mod.rs |
Updates module-level documentation wording. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ProllyTree's history-independence guarantee — two trees with the same final
(key, value)set should have the same root hash regardless of operationorder — was broken at multiple layers. This PR implement streaming-chunker
design to Rust and makes the property hold by construction, then sweeps a
test matrix at every consumed API layer to keep it that way.
What changed
src/streaming_chunker.rs:Splitter(rolling-hash, resets at everychunk boundary),
NodeBuilder,Chunker,NodeCursor, andapply_mutations.Splitter reset-on-boundary is what makes the algorithm history-independent:
a chunk's split decision depends only on items inside that chunk.
ProllyTree::{insert, delete, insert_batch, delete_batch}now routethrough
apply_changes→apply_mutations. The legacy in-placeBalanced::balancepath is no longer on the public mutation path.GitVersionedKvStore::commitandGitNamespacedKvStore::commit_impldrain their staging areas through
apply_changesin a single batch insteadof N per-item canonicalizations.
try_pure_append(when every mutationis past the tree's max key) and
fast_forward_to_end(alignment-aware skiponce the chunker re-syncs with the old tree's chunk hash).
Test coverage added
24 new tests across 4 files, all assert root-hash equality across orders:
src/streaming_chunker.rsProllyTreeAPIsrc/tree.rs::history_independence_testssrc/tree.rs::merge_canonicality_testsGitVersionedKvStoresrc/git/versioned_store/tests.rsGitNamespacedKvStoresrc/git/versioned_store/namespaced_tests.rstests/history_independence.rsPerformance
cargo bench --bench tree -- insert_singleat N=10 000:The remaining gap to baseline is the inherent O(N²) of one-mutation-per-call;
batched mutations via
apply_changesare near-linear.Backward compatibility
produces a canonical (different) root hash. Data is preserved; only the
tree's internal shape may shift. For
git-prolly, a single no-op commitisolates the one-time re-canonicalization diff.