test(parquet): proptest the compaction row-conservation invariant (RFC0009.2) - #120
Conversation
|
@coderabbitai review |
|
Warning Review limit reached
More reviews will be available in 47 minutes and 46 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 ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a property-based test to validate compaction row-conservation. It introduces the ChangesCompaction row-conservation property test
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
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.
|
cc912f4 to
a6c326c
Compare
There was a problem hiding this comment.
Pull request overview
Adds a proptest-based property test to ourios-parquet to validate RFC0009.2’s compaction invariant: compaction must conserve the full multiset of rows (count + content) across arbitrary splits of records into multiple input files.
Changes:
- Introduce
compaction_conserves_every_rowproperty test that writes 2–5 Parquet files, runscompact_partition, then asserts row-count and record-multiset equality after reading back the consolidated output. - Add
proptestas a dev-dependency forourios-parquet.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/ourios-parquet/src/compaction.rs | Adds a property test asserting compaction preserves every stored row across randomized multi-file inputs. |
| crates/ourios-parquet/Cargo.toml | Adds proptest under dev-dependencies to support the new property test. |
💡 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.
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-parquet/src/compaction.rs`:
- Around line 680-691: The proptest generators in compaction.rs currently use
half-open ranges (1..15usize and 2..5usize) which produce 1–14 records and 2–4
files; update those ranges in the proptest::collection::vec calls to inclusive
bounds (use 1..=15usize for records and 2..=5usize for files) so the test covers
1–15 records and 2–5 files as described; locate the proptest::collection::vec
invocation in compaction.rs and change the two range literals accordingly.
🪄 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: 9e691513-8737-4bc3-87da-87956dbe7538
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/ourios-parquet/Cargo.tomlcrates/ourios-parquet/src/compaction.rs
…C0009.2) Epic #94 leftover. Compaction's core promise (RFC0009.2 — "preserving every stored row") had only example-based coverage. Add a property test: for any split of records across ≥2 files in one partition, the consolidated file holds exactly the same multiset of rows — count and content (reordering aside). The strategy varies the fields the invariant exercises (template id, in-hour timestamp, severity, one param value) on the clean-round-trip record shape, so equality reflects compaction, not codec edge cases. It writes each file via the real Writer, runs compact_partition, reads the consolidated file back via Reader::open_partition, and asserts row count == input and (sorted) full-record equality. Introduces `proptest` as an ourios-parquet dev-dependency (CLAUDE.md §6.2 mandates property tests for invariants; this is the first adopter). Capped at 48 cases (each builds + compacts + re-reads a store) to keep the suite fast (~3s). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
a6c326c to
5e960e1
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
What
First of the epic #94 compaction leftovers: a property test for row conservation (RFC0009.2), compaction's core promise — "preserving every stored row."
Why
compact_partitiononly had example-based tests (2 files → fixed rows). The invariant — any set of records across any split of files comes back exactly — deserves a property test (CLAUDE.md §6.2: "property tests for anything with an invariant... the Parquet writer").How
compaction_conserves_every_row: generate 2–5 files of 1–15 records each (varying template id, in-hour timestamp, severity, one param value over the clean-round-trip record shape), write them via the realWriter,compact_partition, then read the consolidated file back viaReader::open_partitionand assert:outcome.rows == total input count, and==, so content is preserved, not just the count).Introduces
proptestas anourios-parquetdev-dependency (first adopter of the §6.2-mandated tool). Capped at 48 cases — each builds + compacts + re-reads a store — keeping the test ~3s.Verification
cargo test --workspace,cargo clippy --workspace --all-targets --all-features -D warnings,cargo fmt --all --checkall green. The property held across all cases.Part of epic #94. Follow-ups: the compaction crash-recovery / atomic-swap test (RFC0009.3) and the
ourios.compaction.backlogUpDownCounter.🤖 Generated with Claude Code
Summary by CodeRabbit