feat(bench): land A1 compression-ratio measurement (PR-I2) - #52
Merged
Conversation
Second implementation slice on RFC 0006. The A1 thesis gate
("Ourios beats ZSTD-alone by ≥ 3× on the compression ratio")
now runs end-to-end. C2 remains NotImplemented; A1 and C1 can
run individually or together in a single miner pass.
Lands:
- `src/a1.rs` — `A1Accumulator` streams emitted records into
per-partition `ourios_parquet::Writer`s during the harness
loop (memory bounded at ~one row group per open partition),
captures the miner's audit-event stream into the `audit/...`
series, then at finalize closes the writers, sums the
on-disk `*.parquet` bytes (data + audit, skipping
`*.parquet.tmp`), runs the ZSTD-19 reference codec over each
`*.txt` individually, and computes the §3.4.1 ratios. The
delta is floored to three significant figures so reported
numbers err pessimistic.
- `src/harness.rs` — wires a `SharedAuditSink` alongside the
record sink and returns the drained audit events
(`HarnessResult`). The internal snapshot map is no longer
surfaced (no consumer past the loop).
- `src/lib.rs` — `run()` restructured to set up whichever
gate accumulators are enabled and feed both from one
callback; resolves the Parquet output bucket (caller-
supplied or a scratch `TempDir`, persisted on
`--keep-parquet`).
- `crates/ourios-parquet/src/partition.rs` — `PartitionKey`
derives `Hash` so the bench can group records / audit
events by partition in a `HashMap`. Purely additive.
- `tests/a1.rs` — RFC0006.1 un-`#[ignore]`'d; the real
`zstd_level_19_bytes` helper (via the `zstd` crate) replaces
the stub. The test recomputes every byte count from disk
and asserts the bench's reported values match.
ZSTD: uses the `zstd` crate (ergonomic safe wrapper over
`zstd-safe`, already in the tree via parquet's `zstd`
feature) per the RFC §7 resolution — same bundled C library,
version pinned by Cargo.lock, no host `zstd` binary needed.
Test surface:
- `src/a1.rs` unit tests: 3-sigfig floor, the ratio-of-ratios
formula, pass/fail threshold, zero-output guard.
- `tests/a1.rs`: both RFC0006.1 scenarios now green against
the seed corpus (formula legs verified against independent
disk recomputation; actual §9 pass/fail numbers are a
separate benchmarking session per the RFC).
- The lib.rs marker test narrows to `c2_still_returns_not_implemented`.
- The harness's snapshot-map test is replaced by
`repeated_template_reuses_one_snapshot` (the map is no
longer a public output; the reuse contract is what matters).
Verification (CLAUDE.md §6.6):
- cargo fmt --all --check — clean.
- cargo clippy --all-targets --all-features -- -D warnings —
clean.
- cargo test --all-features — 249 passed / 22 ignored
(was 237 / 24; the two RFC0006.1 A1 tests flipped green).
Maturity gate: RFC 0006 stays `red` until C2, RFC0006.5, and
RFC0006.6 also pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements the RFC 0006 A1 compression-ratio benchmark, enables A1 and C1 in the bench harness (C2 remains unimplemented), updates the harness to capture and return audit events, adds an A1 accumulator that streams per-partition Parquet outputs and measures bytes, integrates A1 into run() with bucket handling, and enables ZSTD‑19 based integration tests. ChangesRFC 0006 A1 Benchmark Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ourios-bench/src/a1.rs (1)
314-322: 💤 Low valueMinor: Inconsistent error variant for ZSTD compression failure.
zstd_level_19_bytesusesBenchError::Reportfor compression errors (line 318) butBenchError::Corpusfor file read errors (line 314). Given that ZSTD compression is part of the measurement pipeline rather than report generation,BenchError::Pipelinewould be more semantically consistent.♻️ Suggested consistency fix
let compressed = zstd::bulk::compress(&bytes, ZSTD_LEVEL).map_err(|e| { - crate::BenchError::Report { + crate::BenchError::Pipeline { detail: format!("zstd compress({}): {e}", path.display()), } })?;🤖 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/a1.rs` around lines 314 - 322, The compression error in zstd_level_19_bytes currently maps zstd::bulk::compress failures to BenchError::Report but should use BenchError::Pipeline for semantic consistency with the pipeline work (file-read errors already use BenchError::Corpus); update the map_err closure that handles zstd::bulk::compress to return crate::BenchError::Pipeline with the same detail string (e.g., format!("zstd compress({}): {e}", path.display())) so compression failures are reported as pipeline errors.
🤖 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/a1.rs`:
- Around line 314-322: The compression error in zstd_level_19_bytes currently
maps zstd::bulk::compress failures to BenchError::Report but should use
BenchError::Pipeline for semantic consistency with the pipeline work (file-read
errors already use BenchError::Corpus); update the map_err closure that handles
zstd::bulk::compress to return crate::BenchError::Pipeline with the same detail
string (e.g., format!("zstd compress({}): {e}", path.display())) so compression
failures are reported as pipeline errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 808734df-8117-4681-a5a2-6827a56bc509
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/ourios-bench/Cargo.tomlcrates/ourios-bench/src/a1.rscrates/ourios-bench/src/harness.rscrates/ourios-bench/src/lib.rscrates/ourios-bench/tests/a1.rscrates/ourios-parquet/src/partition.rs
Contributor
There was a problem hiding this comment.
Pull request overview
Implements RFC 0006’s A1 thesis-gate end-to-end in ourios-bench by streaming Parquet output during a single miner pass, capturing audit events, and computing the on-disk compression-ratio delta against per-file ZSTD-19.
Changes:
- Added
A1Accumulatorto write partitioned Parquet during ingest and compute A1 ratios from on-disk byte counts. - Updated the harness to wire a
SharedAuditSinkand return drained audit events for A1’sbytes(ourios_output)accounting. - Enabled RFC0006.1 A1 integration tests and added
zstdas the reference codec dependency.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-parquet/src/partition.rs | Adds Hash derivation to PartitionKey to support grouping writers by partition. |
| crates/ourios-bench/src/a1.rs | New A1 implementation: streaming Parquet writes, audit Parquet writes, and ZSTD-19 reference measurement. |
| crates/ourios-bench/src/harness.rs | Adds audit sink plumbing and returns drained audit events from the miner run. |
| crates/ourios-bench/src/lib.rs | Wires A1 + C1 in a single pass, adds bucket resolution/TempDir handling, updates gate behavior. |
| crates/ourios-bench/tests/a1.rs | Un-ignores RFC0006.1 and validates reported byte counts/ratios via independent disk recomputation. |
| crates/ourios-bench/Cargo.toml | Adds zstd + tempfile deps for A1 implementation and tests. |
| Cargo.lock | Records the new zstd dependency for ourios-bench. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@crates/ourios-bench/src/a1.rs`:
- Around line 223-232: Add a colocated unit test that verifies the day-level
audit partitioning collapses hours: call the function that produces the
PartitionKey (the code path using PartitionKey::derive via the audit_partition
logic where key.hour = 0) for two proxies/timestamps on the same calendar day
but different hours and assert the resulting PartitionKey values are equal;
place the test in the same module as the implementation and construct inputs so
only the hour differs to prove key.hour normalization works.
In `@crates/ourios-bench/src/lib.rs`:
- Around line 287-296: Replace uses of std::fs::metadata with
std::fs::symlink_metadata so directory tests don't follow symlinks; call
std::fs::symlink_metadata(&path) and if the returned metadata's
file_type().is_symlink() is true, skip recursing on that entry, otherwise test
meta.is_dir() and recurse as before (this applies to the check around the
variable meta and path in lib.rs and the analogous checks in a1.rs). Ensure the
BenchError::Pipeline error message still includes path.display() when
symlink_metadata fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c4d65508-051a-48ed-92fd-10d2a0b13952
📒 Files selected for processing (2)
crates/ourios-bench/src/a1.rscrates/ourios-bench/src/lib.rs
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/ourios-bench/src/lib.rs`:
- Around line 251-264: resolve_bucket currently rejects --keep-parquet
unconditionally which breaks valid C1-only runs because run() still calls
resolve_bucket; update resolve_bucket (or its caller) to first check the A1 gate
(config.gates.a1) and only enforce the "--keep-parquet requires --bucket-dir"
Cli error when A1 is enabled, otherwise ignore keep_parquet (treat it as a
no-op) so C1-only invocations like --gates c1 --keep-parquet do not error; touch
the resolve_bucket function and/or run() call site to gate the validation on
config.gates.a1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a5ff1ad-b845-4487-9d36-34944905acf0
📒 Files selected for processing (3)
crates/ourios-bench/src/a1.rscrates/ourios-bench/src/lib.rscrates/ourios-bench/tests/a1.rs
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/ourios-bench/src/a1.rs (2)
359-395: 💤 Low valueCorpus walker follows symlinks; could loop on a symlinked directory.
zstd_level_19_bytesusesstd::fs::metadata(Line 372), which follows symlinks. A corpus directory containing a symlink pointing to an ancestor would cause an infinite loop. The bucket scanners (sum_parquet_bytes) usesymlink_metadatato avoid this.This is likely low risk since the corpus is controlled input per RFC 0006 §3.3, but for consistency with the bucket scanners, consider using
symlink_metadataand skipping symlinks here too.🤖 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/a1.rs` around lines 359 - 395, In zstd_level_19_bytes, stop following symlinks by replacing the std::fs::metadata(&path) call with std::fs::symlink_metadata(&path) (in function zstd_level_19_bytes) and then skip any entry where meta.file_type().is_symlink(); use meta.file_type().is_dir() to detect directories to push on the stack and treat non-dir, non-symlink files as before. This matches the bucket scanners' behavior and prevents looping on symlinked directories.
104-127: 💤 Low valueDouble clone on partition miss; entry API would be simpler and clone once.
The comment claims
contains_key+get_mutavoids clones on the hot path, but on a miss you clonepartitiontwice (Line 111 forWriter::open, Line 116 forinsert). TheEntryAPI withor_insert_withwould clone only once and eliminate the double lookup:♻️ Suggested simplification
- if !self.data_writers.contains_key(&partition) { - let writer = Writer::open(&self.bucket_root, partition.clone()).map_err(|e| { - crate::BenchError::Pipeline { - detail: format!("parquet writer open: {e}"), - } - })?; - self.data_writers.insert(partition.clone(), writer); - } - let writer = self - .data_writers - .get_mut(&partition) - .expect("writer inserted above when absent"); + use std::collections::hash_map::Entry; + let writer = match self.data_writers.entry(partition.clone()) { + Entry::Occupied(e) => e.into_mut(), + Entry::Vacant(e) => { + let w = Writer::open(&self.bucket_root, e.key().clone()).map_err(|e| { + crate::BenchError::Pipeline { + detail: format!("parquet writer open: {e}"), + } + })?; + e.insert(w) + } + };🤖 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/a1.rs` around lines 104 - 127, The code clones `partition` twice on a miss; replace the contains_key/insert/get_mut pattern with the HashMap Entry API to clone once: use self.data_writers.entry(partition.clone()) and match Entry::Vacant to call Writer::open(&self.bucket_root, partition.clone()) once, insert the created writer via vacant.insert(writer) (propagating the error from Writer::open with the same BenchError mapping), then use entry.get_mut()/or call self.data_writers.get_mut(&partition) afterwards to append_records; reference: data_writers, partition, Writer::open, insert, get_mut, and the Entry API (Vacant/insert).
🤖 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/a1.rs`:
- Around line 359-395: In zstd_level_19_bytes, stop following symlinks by
replacing the std::fs::metadata(&path) call with
std::fs::symlink_metadata(&path) (in function zstd_level_19_bytes) and then skip
any entry where meta.file_type().is_symlink(); use meta.file_type().is_dir() to
detect directories to push on the stack and treat non-dir, non-symlink files as
before. This matches the bucket scanners' behavior and prevents looping on
symlinked directories.
- Around line 104-127: The code clones `partition` twice on a miss; replace the
contains_key/insert/get_mut pattern with the HashMap Entry API to clone once:
use self.data_writers.entry(partition.clone()) and match Entry::Vacant to call
Writer::open(&self.bucket_root, partition.clone()) once, insert the created
writer via vacant.insert(writer) (propagating the error from Writer::open with
the same BenchError mapping), then use entry.get_mut()/or call
self.data_writers.get_mut(&partition) afterwards to append_records; reference:
data_writers, partition, Writer::open, insert, get_mut, and the Entry API
(Vacant/insert).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ab22c367-639d-46eb-bf42-e99c5e04d365
📒 Files selected for processing (4)
crates/ourios-bench/src/a1.rscrates/ourios-bench/src/c1.rscrates/ourios-bench/src/harness.rscrates/ourios-bench/src/lib.rs
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+205
to
+208
| let file = std::fs::File::open(&path).expect("open txt"); | ||
| let mut compressed = Vec::new(); | ||
| zstd::stream::copy_encode(file, &mut compressed, 19).expect("zstd compress"); | ||
| *total += compressed.len() as u64; |
Comment on lines
+377
to
+380
| let meta = std::fs::metadata(&path).map_err(|e| crate::BenchError::Corpus { | ||
| detail: format!("metadata({}): {e}", path.display()), | ||
| })?; | ||
| if meta.is_dir() { |
Comment on lines
+287
to
+294
| let entries = match std::fs::read_dir(&d) { | ||
| Ok(entries) => entries, | ||
| Err(e) if e.kind() == std::io::ErrorKind::NotFound => continue, | ||
| Err(e) => { | ||
| return Err(BenchError::Pipeline { | ||
| detail: format!("scan bucket {}: {e}", d.display()), | ||
| }); | ||
| } |
Comment on lines
+155
to
+157
| Ok(HarnessResult { | ||
| audit_events: audit_sink.map(|s| s.drain()).unwrap_or_default(), | ||
| }) |
This was referenced May 26, 2026
14 tasks
This was referenced Jul 10, 2026
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
Second implementation slice on RFC 0006. The A1 thesis gate ("Ourios beats ZSTD-alone by ≥ 3× on the compression ratio") now runs end-to-end. C2 stays
NotImplemented; A1 and C1 run individually or together in a single miner pass.Lands
src/a1.rs—A1Accumulator:ourios_parquet::Writers during the harness loop (memory bounded at ~one row group per open partition — no buffering the full corpus).audit/...series (RFC §3.4.1 requiresbytes(ourios_output)to include audit).*.parquetbytes (data + audit, skipping*.parquet.tmpper RFC 0005 §7), runs ZSTD-19 over each*.txtindividually, computes the §3.4.1 ratios.deltafloored to 3 sig-figs so reported numbers err pessimistic.src/harness.rs— wires aSharedAuditSinkand returns drained audit events (HarnessResult). The internal snapshot map is no longer surfaced (no consumer past the loop).src/lib.rs—run()sets up whichever gate accumulators are enabled and feeds both from one callback; resolves the Parquet output bucket (caller-supplied or scratchTempDir, persisted on--keep-parquet).crates/ourios-parquet/src/partition.rs—PartitionKeyderivesHashso the bench can group by partition in aHashMap. Purely additive.tests/a1.rs— RFC0006.1 un-#[ignore]'d; realzstd_level_19_bytes(via thezstdcrate) replaces the stub. The test recomputes every byte count from disk and asserts the bench's reported values match.ZSTD choice
Uses the
zstdcrate — the ergonomic safe wrapper overzstd-safe, already in the tree via parquet'szstdfeature — per the RFC §7 resolution. Same bundled C library, version pinned byCargo.lock, no hostzstdbinary needed.Test plan
cargo fmt --all --check— clean.cargo clippy --all-targets --all-features -- -D warnings— clean.cargo test --all-features— 249 passed / 22 ignored (was 237 / 24; the two RFC0006.1 A1 tests flipped green).src/a1.rsunit tests: 3-sigfig floor, ratio-of-ratios formula, pass/fail threshold, zero-output guard.tests/a1.rsboth RFC0006.1 scenarios green against the seed corpus — formula legs verified against independent disk recomputation. Actual §9 pass/fail numbers are a separate benchmarking session per the RFC (the seed corpus is tiny; Parquet footer/dictionary overhead dominates, so its delta isn't representative).Note for reviewers
snapshot_map_covers_…test was replaced (not silently dropped) byrepeated_template_reuses_one_snapshot— the snapshot map is no longer a public harness output, so the test now pins the reuse contract through the callback instead. Flagging per the "don't weaken tests without surfacing it" discipline.a1_and_c2_still_return_not_implementedtoc2_still_returns_not_implemented(A1 graduated).Maturity stage
RFC 0006 stays
reduntil C2, RFC0006.5, and RFC0006.6 also pass.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Tests
Chores
Style / Misc