Repository navigation
test(bench): add unattached branch root-write scenarios - #961
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
ragnorc
left a comment
There was a problem hiding this comment.
Approve the benchmark change at d7f467fdbd23c4a3dc144713a973e1981a1d384d. I found no blocking code defect. The separate DST failure below still needs to be resolved before merge.
This PR supplies repeatable inputs for measuring an unattached root write. It adds a tiny insert, an update after 32 published updates, and a new root beside 32 retained unattached snapshots. Each workload checks the new snapshot, checks that main keeps its rows, and drops the measured snapshot after the timed operation. The history workload also checks the snapshot after reopening. These inputs let later runs compare costs; this PR supplies no speedup result.
The evidence matches the stated boundary:
- The selected operation hooks enclose the engine mutation. Assertions, snapshot reads, restart, and drop run afterward. A failed verification rejects the sample. The existing prefix rules also prevent a prerequisite write from being hidden before the selected operation.
- The retained-chain check verifies all 32 live snapshots, one root, exact parent links, and consecutive generations. It compares the full chain and published inventory before and after the workload. Thus 32 independent roots, an advanced
main, or loss of a retained child cannot satisfy this fixture check. - The tiny and published-history cases reuse their ordinary-write fixtures. The retained-chain case is a different storage-history treatment. It measures a new root beside that chain, not an extension of its tip. It cannot alone isolate the cost of publication or establish chained-write throughput. The README states the root-only and merge-only attribution limits.
I traced the mutation through unattached staging, the detached overwrite, and snapshot drop. I checked the complete relevant Lance guides and format documentation against the pinned implementation: Lance 11.0.0, source ab6b5bbe46009ed78746b444df8db59a8bc5d842. Its detached commit path writes a detached manifest; native tags retain its address. Dropping a snapshot removes its reference. It does not establish physical space reclamation. The new cases make no cleanup-cost claim.
The maintenance tradeoff is favorable. The assertions reduce the risk that fixture drift silently changes a measured treatment. The PR adds fixture and assertion code, but no production state, public API, storage format, or timing abstraction. Sharing fixtures and the history workload limits drift. The explicit 32-step recipe costs maintenance, but it fits the existing rule that commit bindings cannot appear in loops. Five similar additions can still use the same catalog and runner. There is no need to expand the GQT language for this change.
I also surveyed the other open PRs. The cache work in #818/#825 and group-commit direction in #785 may change these costs. The existing source/engine identity keeps those runs distinct; these root-write cases do not qualify those separate execution paths. Current upstream Lance 13.0.0 and 14.0.0 beta are future test targets, not versions validated here. This PR adds no Lance adapter to carry through an upgrade.
Validation:
- Local, clean isolated checkout at the exact head above:
git diff --check d334dc3c0510481a2484ad0ad0b65d1b23c99d8c HEADpassed. - Local command:
RUSTUP_TOOLCHAIN=1.97.1 CARGO_TARGET_DIR=/tmp/review-928/target RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable' RUST_MIN_STACK=16777216 RUSTC_WRAPPER= cargo test -p omnigraph-bench --locked --lib gqt_tests:: -- --test-threads=1— 103 passed, 0 failed, 0 ignored, 306 filtered out; test execution took 46.60 seconds. This includes both new history tests and the tiny mutation case in the receipt test. The build reused the existing target cache. - This is new benchmark coverage, not a behavior fix. The comparison base is
d334dc3c0510481a2484ad0ad0b65d1b23c99d8c; the reviewed head is above. A before/after failing bug regression is not applicable. I made no temporary source or test edits. - CI and GQT passed. CI checked merge commit
5c9a358232519fe8d42307066269679318ab7681, whose tree matches this head (b8204aa49e1b1d7f58ddbbfc317217374b77b777). - DST pinned suite failed: its
scenariosexecutable reported 50 passed, 1 failed, 28 ignored.dst_schema_apply_racing_writers_first_contact, seed24301, failed its schedule-coverage assertion atscenarios.rs:4826: no schema apply landed between two writer commits. This PR changes neither that test nor production code. That observation does not prove the failure is harmless; resolve it or obtain a passing rerun before merge.
Local checks exercise the benchmark inputs and adapter on small tables with fixed payloads. I did not acquire release performance records, test these cases through HTTP or cloud storage, or validate concurrent, large-payload, or many-table workloads. No optional code changes requested.
What & why
This PR adds three unattached root-write scenarios to the benchmark catalog.
tiny-mutatefixture.Each scenario verifies snapshot contents, preserves
main, and drops the measured snapshot outside measurement.Backing issue / RFC
Checklist
benchmarks/README.mdwith scenarios and measurement limits.changelog.d/unattached-branch-benchmarks.added.md.docs/dev/invariants.md; no Hard Invariant weakened or deny-list item hit. Nearest invariant: evidence matches the boundary.Local verification
env RUST_MIN_STACK=16777216 cargo test -p omnigraph-bench --locked --lib gqt_tests:: -- --test-threads=1: 103 passed. The default-stack run aborted in unchangedbounded_hot_table_idle_base.cargo run -q -p omnigraph-vocabulary-guard -- check --surface rust-string --base c92b267d6153f4342e4d7c03c26b8711f7a79ab1 --inventory tools/omnigraph-vocabulary-guard/graph-vocabulary-inventory.tsv --openapi openapi.json: failed on the existing merge-base inventory. The changed Rust file has identical base/current scans with zero observations; the existing CI audit is disabled.Notes for reviewers