feat(bench): add B2 query-latency criterion bench (synthetic + otel-demo) - #92
Conversation
…emo) Supportive, non-gating wall-clock evidence for the RFC0007.2 thesis gate that ourios-querier already proves structurally. New criterion target `crates/ourios-bench/benches/b2.rs` with two groups: - `b2/synthetic` (always runs): result size held constant (TARGET_ROWS of one template) while the corpus scales 1x/10x/50x with filler under distinct templates in separate files. Isolates the B2 variable — latency vs corpus at fixed result. - `b2/otel-demo` (runs when OURIOS_B2_CORPUS_DIRS is set, a comma-separated list of corpus dirs): loads → mines → writes each real corpus to a temp Parquet store and times a query for the busiest template. Skipped when unset (the corpora aren't committed; CI / an operator stages them). To query a real corpus the bench needs the mined records as a queryable store, so `ourios_bench::build_query_store` (new public `src/store.rs`) reuses the existing corpus loader + miner harness (the same pipeline A1 measures) and writes per-partition Parquet. criterion + ourios-querier (DataFusion) + a current-thread tokio runtime are dev-deps under the `[[bench]]` target, so the bench binary stays out of the A1/C1/C2 harness. Indicative finding (laptop, not §1 baseline, so not recorded in §9): synthetic latency is sub-linear in corpus (50x corpus -> ~4.6x latency at constant result) — the residual is per-file footer/ metadata reads (file count scales with corpus), not data scanning (which the structural B2 test proves flat). That points at the small-file/compaction hazard (§4 #4), not a thesis failure. benchmarks.md B2 section documents the instrument + how to run it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a B2 Criterion benchmark measuring template-exact query latency versus corpus size, implements a Parquet store builder (BuiltStore, build_query_store, append_record) with bucket-parquet detection, wires dev-dependencies and a ChangesB2 Template-Exact Query Latency Benchmark
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new B2 (template-exact) query-latency Criterion benchmark to the ourios-bench crate, providing non-gating wall-clock evidence to complement the existing deterministic (structural) B2 proof in ourios-querier.
Changes:
- Introduces a new
b2Criterion bench with two groups: a synthetic corpus-scale experiment and an opt-inotel-democorpus run controlled byOURIOS_B2_CORPUS_DIRS. - Adds a reusable public helper
build_query_storeto load→mine→write a corpus into an RFC0005 Parquet store suitable forourios-querierqueries. - Updates benchmark documentation to reflect the new B2 instrumentation and current measurement status.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/benchmarks.md | Documents B2’s structural proof and the new Criterion latency instruments; updates query-side status narrative. |
| crates/ourios-bench/src/store.rs | Adds build_query_store + BuiltStore to materialize a queryable Parquet store from a corpus. |
| crates/ourios-bench/src/lib.rs | Exposes the new store builder from the ourios-bench library API. |
| crates/ourios-bench/Cargo.toml | Adds dev-deps for Criterion + querier + tokio and registers the b2 bench target. |
| crates/ourios-bench/benches/b2.rs | Implements the new Criterion B2 bench (synthetic + optional real corpora). |
| Cargo.lock | Locks new dependencies introduced by the Criterion bench/dev-deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ourios-bench/src/store.rs (1)
52-96: ⚡ Quick winAdd colocated unit tests for
build_query_store.This module has no
#[cfg(test)]block, butbuild_query_storeis non-trivial: row counting, per-partition writer lifecycle, deferred-error propagation, and busiest-template selection are all regression-prone. A small test that mines a tiny temp corpus and assertsBuiltStorefields (rows, files, busiest id/rows) would pin these contracts cheaply.As per coding guidelines: "Unit tests must be colocated next to the code for anything non-trivial".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-bench/src/store.rs` around lines 52 - 96, Add a colocated #[cfg(test)] mod with unit tests for build_query_store that exercise row counting, per-partition writer lifecycle, deferred-error propagation, and busiest-template selection: create temporary corpus and bucket_root Path(s) (use tempdir/tempfile), invoke build_query_store(corpus_dir, bucket_root) and assert the returned BuiltStore fields (rows, files, busiest_template_id, busiest_template_rows) match expected values for a tiny mined corpus; include at least one test that produces a write error to verify first_err propagation (e.g., by making bucket_root non-writable or injecting a failing Writer) and assert Err is returned, and clean up temp dirs. Ensure the module references build_query_store, writers/counts behavior, and harness::run flow when locating code to test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/ourios-bench/src/store.rs`:
- Around line 52-96: Add a colocated #[cfg(test)] mod with unit tests for
build_query_store that exercise row counting, per-partition writer lifecycle,
deferred-error propagation, and busiest-template selection: create temporary
corpus and bucket_root Path(s) (use tempdir/tempfile), invoke
build_query_store(corpus_dir, bucket_root) and assert the returned BuiltStore
fields (rows, files, busiest_template_id, busiest_template_rows) match expected
values for a tiny mined corpus; include at least one test that produces a write
error to verify first_err propagation (e.g., by making bucket_root non-writable
or injecting a failing Writer) and assert Err is returned, and clean up temp
dirs. Ensure the module references build_query_store, writers/counts behavior,
and harness::run flow when locating code to test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a4f539e2-ca15-4be2-abe3-debaf27f2cad
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/ourios-bench/Cargo.tomlcrates/ourios-bench/benches/b2.rscrates/ourios-bench/src/lib.rscrates/ourios-bench/src/store.rsdocs/benchmarks.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ourios-bench/src/lib.rs (1)
316-372: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd colocated unit tests for
find_published_parquet.This function is non-trivial (56 lines with directory traversal, symlink handling, and multiple error paths) and lacks colocated unit tests. As per coding guidelines, unit tests must be colocated for anything non-trivial in
crates/**/src/**/*.rs.Suggested test cases:
- Empty bucket (no
data/oraudit/directories) → returnsOk(None)- Parquet file in
data/subtree → returnsOk(Some(path))- Parquet file in
audit/subtree → returnsOk(Some(path))- Parquet in nested subdirectory → found correctly
- Symlink in bucket → skipped without error
- Bucket with non-parquet files → returns
Ok(None)As per coding guidelines:
crates/**/src/**/*.rs: Unit tests must be colocated next to the code for anything non-trivial🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-bench/src/lib.rs` around lines 316 - 372, Add colocated unit tests for find_published_parquet in the same file inside a #[cfg(test)] mod; create temporary directories (via tempfile::tempdir or std::env::temp_dir + unique dirs) to simulate a bucket root and programmatically create/omit data/ and audit/ subtrees, nested directories, regular non-parquet files, a .parquet file in data/ and audit/ (and nested), and a symlinked entry to ensure symlinks are skipped; for each scenario call find_published_parquet and assert the Result is Ok(None) or Ok(Some(path)) as appropriate, and include tests for error paths where a missing directory yields Ok(None) and that symlinked parquet is not returned. Ensure tests reference the function find_published_parquet and clean up temporary files after assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/ourios-bench/src/lib.rs`:
- Around line 316-372: Add colocated unit tests for find_published_parquet in
the same file inside a #[cfg(test)] mod; create temporary directories (via
tempfile::tempdir or std::env::temp_dir + unique dirs) to simulate a bucket root
and programmatically create/omit data/ and audit/ subtrees, nested directories,
regular non-parquet files, a .parquet file in data/ and audit/ (and nested), and
a symlinked entry to ensure symlinks are skipped; for each scenario call
find_published_parquet and assert the Result is Ok(None) or Ok(Some(path)) as
appropriate, and include tests for error paths where a missing directory yields
Ok(None) and that symlinked parquet is not returned. Ensure tests reference the
function find_published_parquet and clean up temporary files after assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6462707d-d569-49db-9a7d-504d80e0137b
📒 Files selected for processing (2)
crates/ourios-bench/src/lib.rscrates/ourios-bench/src/store.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/ourios-bench/src/store.rs
* docs(rfc-0009): draft background compaction RFC Opens RFC 0009 at `drafted` — the post-MVP compaction RFC that RFC 0005 §4.5 deferred and hazard H4 requires. Consolidates a sealed partition's small *.parquet files into RFC 0005 §3.5-sized files without changing any stored row, hosted as a background per-tenant task in the ingester (compaction module in ourios-parquet; no new crate). Directly motivated by the PR #92 B2 latency bench: query latency there is dominated by per-file footer reads, not data scanning, so fewer/larger files is the next query-latency lever. The load-bearing open question (flagged for `specified`) is the atomic-publish protocol — §3.4 recommends a per-partition manifest with an atomic generation swap so a query never double-counts or misses a row, which interacts with the RFC 0007 read path and is a small additive RFC 0005 artifact. §5 acceptance criteria (row conservation, query atomicity, crash safety, tenant isolation, forward-compatible merge, sub-linear file count) and §6 testing are a first cut. Wired into SUMMARY.md; mdbook builds, Mermaid renders. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fixup! docs(rfc-0009): draft background compaction RFC --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
What
Stands up the B2 query-latency
criterionbench (RFC 0007 §6) — supportive, non-gating wall-clock evidence for the RFC0007.2 thesis gate thatourios-querieralready proves structurally (deterministically) in its test suite (#89). New[[bench]]targetcrates/ourios-bench/benches/b2.rs.Two groups
b2/synthetic(always runs) — the controlled instrument. Result size held constant (TARGET_ROWSof one template) while the corpus scales 1×/10×/50× with filler under distinct templates in separate files (own row groups, pruned bytemplate_idstats). Isolates the B2 variable: latency vs corpus at fixed result.b2/otel-demo(runs whenOURIOS_B2_CORPUS_DIRSis set — a comma-separated list of corpus dirs) — loads → mines → writes each real corpus to a temp Parquet store and times a query for the busiest template. Skipped when unset (thecorpus/otel-demo-v*dirs aren't committed; CI / an operator stages them and points the env var at them).Run:
cargo bench -p ourios-bench --bench b2.New public API
To query a real corpus the bench needs the mined records as a queryable store, so
ourios_bench::build_query_store(newsrc/store.rs) reuses the existing corpus loader + miner harness (the same pipeline A1 measures) and writes per-partition Parquet — no pipeline duplication. Returns aBuiltStoresummary (rows, files, busiest template) so the bench picks a populated query.criterion+ourios-querier(DataFusion) + a current-threadtokioruntime are dev-deps under the[[bench]]target, so the A1/C1/C2 harness binary stays free of DataFusion.Indicative finding (laptop — NOT recorded in §9)
Synthetic latency is sub-linear in corpus (50× corpus → ~4.6× latency at constant result). The residual growth is per-file footer/metadata reads (file count scales with corpus), not data scanning — which the structural B2 test proves flat. That points at the small-file/compaction hazard (
CLAUDE.md§4 #4), not a thesis failure. These are laptop numbers (not the §1 baseline), so they're not written into §9.Invariants / hazards
ourios-querier's Ourios-owned public types (Querier/QueryRequest/QueryResult). No DataFusion type appears.Verification (local)
cargo fmt --all --check✅cargo clippy -p ourios-bench --all-targets --all-features -- -D warnings✅cargo test -p ourios-bench --all-features✅ (52 + harness/integration tests pass)cargo bench -p ourios-bench --bench b2✅ —b2/syntheticruns;b2/otel-demosmoke-tested against the committedtests/data/otlpsamplemdbook build✅Epic: #82.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation