feat(bench): time-windowed b2/otel-demo arm measuring partition pruning - #118
Conversation
|
@coderabbitai review |
|
Warning Review limit reached
More reviews will be available in 18 minutes and 7 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPR adds time-span tracking to ChangesWindowed Query Benchmark
Sequence DiagramsequenceDiagram
participant otel_demo
participant first_hour_window
participant build_query_store
participant BuiltStore
build_query_store->>build_query_store: track min_time_unix_nano, max_time_unix_nano
build_query_store->>BuiltStore: populate time span fields
otel_demo->>otel_demo: probe unwindowed corpus query, record baseline stats
otel_demo->>first_hour_window: call with corpus and baseline row-groups
first_hour_window->>first_hour_window: validate corpus (multi-file, non-zero span)
first_hour_window->>first_hour_window: construct QueryRequest with 1-hour time_range
first_hour_window->>first_hour_window: probe windowed query
first_hour_window->>first_hour_window: assert windowed row-groups < baseline
first_hour_window->>otel_demo: return windowed query
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 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 |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR extends the ourios-bench harness to measure the payoff of RFC 0007 partition-level time-window pruning on the real otel-demo corpus by adding a 1-hour-windowed B2 benchmark arm and exposing the corpus timestamp span needed to pick a realistic window.
Changes:
- Extend
BuiltStoreto trackmin_time_unix_nano/max_time_unix_nanowhile building the queryable Parquet store. - Add a new
b2/otel-demoCriterion arm (corpus-window-1h) that probes a first-hour time window, asserts pruning, logs observed scan stats, and benchmarks the windowed query latency.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/ourios-bench/src/store.rs | Track and expose the corpus time_unix_nano span in BuiltStore for benchmark window selection. |
| crates/ourios-bench/benches/b2.rs | Add a new otel-demo windowed benchmark arm with a probe/assertion intended to guard partition pruning behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5651f78 to
be331d2
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
be331d2 to
2705040
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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/store.rs (1)
172-194: ⚡ Quick winAdd colocated unit test for time span tracking.
The new
min_time_unix_nano/max_time_unix_nanofeature tracks corpus bounds by filtering out zero timestamps — this is non-trivial behavior (especially the all-zero edge case returning(0, 0)). The existing test only validates bucket reuse rejection. Consider adding a test that:
- Verifies min/max are captured from records with varying timestamps.
- Confirms zero timestamps are excluded from bounds.
- Covers the edge case where all timestamps are zero →
(0, 0).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 172 - 194, Add colocated unit tests in the existing tests module in this file that exercise the new min_time_unix_nano and max_time_unix_nano behavior: write a corpus with records having varying non-zero timestamps and assert build_query_store returns a store whose min_time_unix_nano and max_time_unix_nano reflect the non-zero bounds; write a corpus mixing zero and non-zero timestamps and assert zeros are excluded from bounds; and write a corpus where every record has timestamp zero and assert the returned bounds are (0, 0). Use the existing build_query_store helper and BenchError match style to locate failures and keep tests next to the other tests in the same tests module.
🤖 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 172-194: Add colocated unit tests in the existing tests module in
this file that exercise the new min_time_unix_nano and max_time_unix_nano
behavior: write a corpus with records having varying non-zero timestamps and
assert build_query_store returns a store whose min_time_unix_nano and
max_time_unix_nano reflect the non-zero bounds; write a corpus mixing zero and
non-zero timestamps and assert zeros are excluded from bounds; and write a
corpus where every record has timestamp zero and assert the returned bounds are
(0, 0). Use the existing build_query_store helper and BenchError match style to
locate failures and keep tests next to the other tests in the same tests module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8b140de-2f91-4b3e-966e-bc28768a64b0
📒 Files selected for processing (2)
crates/ourios-bench/benches/b2.rscrates/ourios-bench/src/store.rs
2705040 to
80ca105
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
80ca105 to
1869cd0
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
The payoff of #117 (partition-level time pruning), measured on the real frozen otel-demo corpus. The existing unwindowed b2/otel-demo query scans every partition (a template recurs across all hours on real logs — v4: 5/5). A query bounded to the corpus's first hour now reaches DataFusion with only that hour's partition(s); the rest are pruned at the directory level, so the scanned work tracks the WINDOW, not the corpus span. - `BuiltStore` exposes `min/max_time_unix_nano` (the corpus's timestamp span) so the bench can pick a real window. - `b2/otel-demo` adds a `corpus-window-1h` arm: a query over [first hour) that probes + asserts ≥1 partition is pruned before DataFusion, logs the pruning, and benches the windowed latency. Skipped for single-partition corpora. Indicative on v4 (735,377 rows, 5 partitions; laptop, NOT §9): unwindowed: 5/5 row groups, 5.5 MB, 2.9 ms 1h window: 1 row group (4 partitions pruned), 267 KB, 1.3 ms → ~20× less data read, the inverted-index-collapse locality on real logs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1869cd0 to
3e1931b
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
What
The payoff of #117 (partition-level time pruning), measured on the real frozen otel-demo corpus: a time-windowed
b2/otel-demoarm that demonstrates the inverted-index-collapse locality on real logs.Why
The existing unwindowed
b2/otel-demoquery scans every partition — a template recurs across all hours on real logs (v4: 5/5 row groups). A query bounded to a time window should instead reach DataFusion with only the in-window partition(s); #117 made that possible. This arm measures it.How
BuiltStoreexposesmin/max_time_unix_nano(the corpus's timestamp span) so the bench can pick a real window.b2/otel-demogains acorpus-window-1harm: a query over the corpus's first hour that probes + asserts ≥1 partition is pruned before DataFusion, logs the pruning, and benches the windowed latency. Skipped for single-partition corpora (nothing to prune).Indicative result on v4 (735,377 rows, 5 partitions; laptop, NOT §9)
~20× less data read — the time window bounds the scan to its partitions, not the corpus. (Latency improves less, ~2.2×, because at this scale the ~1 ms fixed per-query DataFusion overhead still dominates; the bytes-read pruning is what scales to GiB/TiB. v1 = single partition → arm skipped.)
This is the real-corpus wall-clock echo of the deterministic
rfc0007_2_time_window_prunes_whole_partitionstest from #117.Verification
cargo fmt --all --check,cargo clippy --workspace --all-targets --all-features -D warnings,cargo test --workspaceall green; validated against the fetchedcorpus/otel-demo-v1+-v4release assets.Part of epic #81 / #82. Follow-ups (not here): wire
b1/otel-demo(severity + window), and a CI query-bench workflow thatgh release downloads the corpus and runs b1/b2.🤖 Generated with Claude Code
Summary by CodeRabbit