feat(bench): RFC 0031 — result-set equivalence comparator (RFC0031.1 core) - #467
Conversation
…core) First increment of the equivalence-harness slice. The result-set equivalence check is the integrity gate every L-gate is fenced behind (RFC0031.1): a bytes/latency comparison between two queries that return different answers is meaningless, so both systems must be proven to answer the SAME question before any metric is trusted. Lands the pure comparison core (no Loki container yet — that is the next increment that flips the RFC0031.1 stub green): - `LineKey` (timestamp_unix_nanos, body) and `AggKey` (bucket, group). - `compare_lines` — MULTISET-exact (per-key counts must match, so 3 vs 2 duplicate identical lines is a mismatch, not a silent pass). - `compare_aggregations` — `(bucket, group_key) -> count` map equality for the L4 class; generic over the map hasher. - `EquivalenceOutcome` / `Mismatch` carry the count-delta summary + bounded example keys the harness writes to stderr on mismatch. Placement stays in ourios-bench (not a new crate) — §7 open. Six unit tests cover order-independence, the duplicate-count catch, one-sided keys, the example cap, and aggregation cells. fmt + clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an RFC0031 comparative harness that checks log-line multisets and aggregation cell maps, reports bounded deterministic mismatch examples, tests the behavior, and publicly re-exports the APIs. ChangesComparative equivalence harness
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Implements the RFC 0031.1 “equivalence-harness” core comparator in ourios-bench, providing deterministic result-set equivalence checks (multiset line comparison and aggregation cell map comparison) that future Loki-vs-Ourios benchmarks can gate on before recording performance metrics.
Changes:
- Added
comparativemodule withLineKey/AggKeykeys andcompare_lines/compare_aggregationsequivalence comparators plus mismatch reporting types. - Exported the new comparator API from
ourios-bench’slib.rsfor use by the harness and follow-on slices. - Included unit tests validating multiset semantics, duplicate-count mismatches, example capping, and aggregation equality/mismatch.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/ourios-bench/src/lib.rs | Wires in and re-exports the new comparative/equivalence comparator API. |
| crates/ourios-bench/src/comparative.rs | Introduces the RFC0031.1 result-set equivalence comparator (lines + aggregations) with tests and mismatch diagnostics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/ourios-bench/src/comparative.rs (2)
141-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider enriching the aggregation summary to match
compare_lines.
compare_linesbreaks the mismatch count intoonly in ourios,only in loki, andunequal counts, butcompare_aggregationsonly reports a total. The same breakdown applies equally to aggregation cells and would help operators diagnose whether a mismatch is a missing cell on one side or a count divergence.♻️ Optional: add breakdown to aggregation summary
differing.dedup_by(|a, b| a.0 == b.0); - let summary = format!("{} aggregation cells differ", differing.len()); + let only_ourios = differing + .iter() + .filter(|(_, o, l)| *l == 0 && *o > 0) + .count(); + let only_loki = differing.iter().filter(|(_, o, _)| *o == 0).count(); + let unequal = differing.len() - only_ourios - only_loki; + let summary = format!( + "{} aggregation cells differ ({only_ourios} only in ourios, {only_loki} only in loki, \ + {unequal} with unequal counts)", + differing.len(), + ); let examples = differing🤖 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/comparative.rs` around lines 141 - 176, Update compare_aggregations to classify differing cells into “only in ourios,” “only in loki,” and “unequal counts,” matching the breakdown produced by compare_lines. Build the summary from these category counts while preserving the existing sorted, deduplicated examples and Equal behavior.
187-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests look solid; consider adding aggregation edge-case coverage.
The six tests cover the key behaviors well. Two gaps worth considering for a future increment:
compare_aggregationswith one-sided keys (present in one map, absent in the other).compare_aggregationswithexamples_capenforcement.These mirror existing
compare_linestests and would guard against regressions in the aggregation path.🤖 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/comparative.rs` around lines 187 - 275, Extend the tests around compare_aggregations with coverage for one-sided aggregation keys and examples_cap enforcement. Add a mismatch test where a cell exists in only one map, and a test with more differing cells than the cap that asserts the examples length is capped while the mismatch summary remains accurate, reusing agg and the existing comparison conventions.
🤖 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/comparative.rs`:
- Around line 141-176: Update compare_aggregations to classify differing cells
into “only in ourios,” “only in loki,” and “unequal counts,” matching the
breakdown produced by compare_lines. Build the summary from these category
counts while preserving the existing sorted, deduplicated examples and Equal
behavior.
- Around line 187-275: Extend the tests around compare_aggregations with
coverage for one-sided aggregation keys and examples_cap enforcement. Add a
mismatch test where a cell exists in only one map, and a test with more
differing cells than the cap that asserts the examples length is capped while
the mismatch summary remains accurate, reusing agg and the existing comparison
conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 62f96f3a-c8c5-440e-a519-f446d54056da
📒 Files selected for processing (2)
crates/ourios-bench/src/comparative.rscrates/ourios-bench/src/lib.rs
- Mark EquivalenceOutcome #[non_exhaustive], matching the crate's convention for evolvable public enums (BenchError, TxtSeverity), so a future variant doesn't break downstream exhaustive matches. - compare_aggregations: two independent hasher params (S1, S2) instead of forcing both maps to share a hasher — the comparison logic doesn't depend on the hasher. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot review: - LineKey.timestamp_unix_nanos / AggKey.bucket_start_unix_nanos are now u64, matching the workspace time_unix_nano representation (record.rs, otlp.rs, store.rs) — these keys come from OTLP's u64 field. - Mismatch examples render bodies through a new body_preview: truncated to 96 bytes with a "+N bytes" suffix, so an arbitrarily large body can't blow up the stderr mismatch report. New unit test covers it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
First increment of the RFC 0031 equivalence-harness green slice: the pure result-set equivalence comparator — the integrity gate every L-gate is fenced behind (RFC0031.1).
A bytes/latency comparison between two queries that return different answers is meaningless, so both systems must be proven to answer the same question before any metric is trusted. This lands the comparison core:
LineKey(timestamp_unix_nanos, body)andAggKey(bucket_start, group_key).compare_lines— multiset-exact: per-key counts must match, so a system returning three identical duplicate lines where the other returns two is a mismatch, not a silent pass.compare_aggregations—(bucket, group_key) → countmap equality for the L4 class; generic over the map hasher.EquivalenceOutcome/Mismatchcarry the count-delta summary + bounded example keys the harness writes to stderr on mismatch.Scope / phasing
This is increment 1 of the slice — the fork-free logic half, verifiable without a container. It does not yet flip the
RFC0031.1stub green; that needs the Loki-testcontainer integration (increment 2) that drives real answers into this comparator. Kept inourios-bench(not a new crate) — the §7 crate-placement question stays open.Tests
Six unit tests: order-independence (multiset ≠ ordered), the duplicate-count catch (3 vs 2), one-sided keys, the example cap, and aggregation-cell equal/mismatch.
cargo test -p ourios-bench --lib comparative→ 6 passed. fmt + workspace clippy clean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests