test(parquet): rfc 0036 green — compaction properties preserved (.3) - #588
Conversation
RFC 0036 slice B / scenario RFC0036.3: the sorted external-merge compaction must preserve RFC 0009's D2/D3 properties and the §3.2 bounded-memory bound. This lands the three checks. - Memory bound (the load-bearing §3.2 claim): a thread-local decoded-row residency gauge (#[cfg(test)] in compaction.rs, instrumenting sort_inputs_into / RunCursor per RFC 0036 §6) drives `rfc0036_3_forced_spill_peak_is_one_input_not_whole_partition`. On a K-input × S-row partition it asserts the two §3.2 paths accurately: forced-spill peak = one input + F×batch (≪ whole partition), and the in-memory skip-spill path holds the whole partition (bounded by in_memory_max_bytes). The gauge is thread-local because a compact_* call runs entirely on its caller's thread, so the assertion is immune to `cargo test` in-process parallelism — and needs no unsafe global allocator (the crate is #![deny(unsafe_code)]). - D3 unchanged: `rfc0036_3_compaction_properties_preserved` (tests/it) extends the rfc0009_1 structural style — tens of input files collapse to exactly one live file per partition, rows conserved, sorting_columns still declared, §3.1 order preserved. The absolute size band is the baseline's job (§9.25), as in rfc0009_1. - D2 band: measured indicatively (local M-series, §9.7 shape) at ~138 MiB/s sorted vs the §9.7 unsorted 166.8 MiB/s (different hardware — not a clean delta; the authoritative sorted-vs-unsorted rerun is a `validated` item). D3 output lands 452.7 MiB, IN the 256 MiB–2 GiB band. Recorded in benchmarks.md §9.25 as a new indicative record; the in-repo RFC0036.3 assertion stays structural (no flaky wall-clock gate). Hazard #4: this pins the §3.2 memory property — that clustering the compacted partition does not regress compaction to whole-partition residency — and confirms the file band (D3) is untouched by the smaller compacted row-group threshold. CLAUDE.md §6.2 property-test discipline: the memory bound is exercised through the real spill path via the internal SortTuning seam, not asserted by inspection. Only the rfc0036_3 stub is replaced; rfc0036_4 (green) and the rfc0036_5 / querier rfc0036_2 stubs are untouched (still ignored, fail-on-todo when force-run). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
|
Warning Review limit reached
Next review available in: 48 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)
📝 WalkthroughWalkthroughChangesRFC 0036 compaction validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…ing comment
The forced-spill peak assertion used one_input + fan_in×SUB_BATCH_ROWS as
the bound, which is vacuous at the test's constants (F×batch=65,536 >
whole partition K×S=72,000) and the comment inverted the sizing ("S ≫
F×batch" — S=12,000 ≪ F×batch). Measured peak is exactly one input
(12,000); phase-2 opens only K<F cursors of small reader batches, well
under S. Replace the vacuous upper bound with a floor (peak ≥ one input,
formation decodes a full input) and keep the < total/2 teeth (fails on a
whole-partition regression); correct the comment to state why F×batch
does not bite here.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
There was a problem hiding this comment.
Pull request overview
Implements the RFC 0036 slice-B “green” work for scenario RFC0036.3, adding structural + instrumentation-backed tests to ensure sorted external-merge compaction preserves RFC 0009’s D2/D3 expectations and the RFC 0036 §3.2 bounded-memory property, and records an indicative benchmark run in docs/benchmarks.md.
Changes:
- Adds a test-only, thread-local “decoded-row residency” gauge in
ourios-parquetcompaction to measure peak decoded-row residency for spill vs in-memory paths. - Replaces the
rfc0036_3integration-test stub with a structural assertion that compaction collapses a partition to exactly one live on-disk parquet file and preserves sorted layout invariants. - Records an indicative D2/D3 measurement entry in
docs/benchmarks.mdfor the sorted compaction path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/benchmarks.md | Adds an indicative benchmark record for D2/D3 after sorted compaction (RFC0036.3). |
| crates/ourios-parquet/tests/it/rfc0036_write_side_layout.rs | Implements RFC0036.3 structural integration test validating one-output-file and sorted layout properties. |
| crates/ourios-parquet/src/compaction.rs | Introduces test-only residency instrumentation and adds the RFC0036.3 memory-bound unit test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… var (review) Two review fixes on #588: the residency gauge's sub() now checked_sub + panics on underflow instead of saturating (an unbalanced add/sub is a bug in the very instrumentation the RFC0036.3 bound relies on, so it must fail loud in test builds, not silently under-report the peak); and the D3 test's loop binds `_` instead of an unused `f` + throwaway `let _ = f;`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
…filter on_disk_parquet_count listed the whole subtree and counted every .parquet; production live_file_keys filters to immediate children of the partition prefix (a nested/sidecar object is not a partition file). Mirror that so the H4 small-file-count assertion stays accurate if the layout ever nests objects under a partition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
![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-parquet/tests/it/rfc0036_write_side_layout.rs (1)
437-472: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated production filtering logic risks silent drift.
on_disk_parquet_count/is_immediate_childhand-copy the immediate-child.parquetfiltering that lives in productioncompaction.rs(used at several call sites there). The commit history for this very PR shows this exact duplication already drifted once ("aligned test file counting with production's immediate-child filtering"), which is a strong signal this is a real maintenance hazard, not a hypothetical one — a future change to the production filter won't automatically flow into this black-box test copy.Consider exposing the production
is_immediate_child/child-counting helper behind a test-support surface (e.g. a#[cfg(any(test, feature = "test-support"))] pubre-export) so this integration test asserts against the real implementation instead of a hand-maintained mirror.🤖 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-parquet/tests/it/rfc0036_write_side_layout.rs` around lines 437 - 472, Remove the duplicated filtering logic from on_disk_parquet_count and is_immediate_child, and expose the production compaction helper or child-counting functionality through the crate’s test-support surface using the existing cfg conventions. Update the integration test to call that shared production implementation while preserving the current behavior of counting only immediate-child .parquet objects.
🤖 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-parquet/tests/it/rfc0036_write_side_layout.rs`:
- Around line 437-472: Remove the duplicated filtering logic from
on_disk_parquet_count and is_immediate_child, and expose the production
compaction helper or child-counting functionality through the crate’s
test-support surface using the existing cfg conventions. Update the integration
test to call that shared production implementation while preserving the current
behavior of counting only immediate-child .parquet objects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cf8e459-729c-4e91-81cf-26e5eef906e8
📒 Files selected for processing (3)
crates/ourios-parquet/src/compaction.rscrates/ourios-parquet/tests/it/rfc0036_write_side_layout.rsdocs/benchmarks.md
…t floor
Per review: the >= one_input floor baked in the current detail that
formation fully decodes an input at once. RFC0036.3's property is an
upper bound ("not whole-partition"); a future formation that streams
within an input could peak below S and still satisfy it. Drop the floor
to a > 0 gauge-liveness sanity, keep the < total/2 teeth and the ×4 <
mem_peak contrast, and rename the test to _far_below_whole_partition.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
RFC 0036 slice B — RFC0036.3 (compaction properties preserved)
Green slice for scenario RFC0036.3: the slice-A sorted external-merge
compaction must preserve RFC 0009's D2 (throughput) / D3 (file band)
and the §3.2 bounded-memory property. Replaces only the
rfc0036_3stub;rfc0036_4(green) and therfc0036_5/ querierrfc0036_2stubs areuntouched (still
#[ignore]d, fail-on-todo when force-run). No change to theingest path, the manifest protocol, or slice-A's merge logic.
Memory bound — the load-bearing §3.2 claim (how it's proven)
compaction::tests::rfc0036_3_forced_spill_peak_is_one_input_not_whole_partitionmeasures peak decoded-row residency via a
#[cfg(test)]thread-localgauge instrumented inside
sort_inputs_into/RunCursor(the counter optionin RFC 0036 §6). On a partition of K=6 inputs × S=12,000 rows it asserts the
two §3.2 paths accurately, on the same fixture:
SortTuning { in_memory_max_bytes: 0 }seam): peak = one input + F×batch — phase 1 decodes inputs strictly one
at a time; phase 2 holds one streamed batch per open run. The test fails if
the merge ever holds the whole partition decoded (
peak < total/2, andpeak ≤ one_input + F·SUB_BATCH_ROWS).in_memory_max_bytes: u64::MAX): peak = the wholepartition (
== K·S), the §7 skip-spill tradeoff, bounded byin_memory_max_bytes. The contrast (spill_peak·4 < mem_peak) is the proof.Why a gauge, not a tracking allocator: the crate is
#![deny(unsafe_code)], soa
#[global_allocator]wrapper (unsafe impl GlobalAlloc) is off-posture; andin the RFC 0028 single
itbinary a process-global peak would be polluted byconcurrent tests. A
compact_*call runs entirely on its caller's thread(blocking I/O throughout), so a thread-local gauge is exact and
parallelism-proof.
D3 unchanged (structural assertion)
rfc0036_3_compaction_properties_preserved(tests/it) extends therfc0009_1structural style: tens of input files → exactly one live file per partition
(physically, GC'd — not manifest-hidden), rows conserved, every row group still
declares
sorting_columns, decoded rows in §3.1 order. The RFC 0036 §3.3smaller row-group threshold does not touch the file band, and D3 measures
files. The absolute 256 MiB–2 GiB size band needs baseline-corpus volume
(§9.25 / §9.7), deferred exactly as
rfc0009_1defers it.D2 throughput band (indicative)
Measured on this PR's branch, local M-series developer machine (P-cores;
taskpolicy -Bto escape the Claude-Code background-QoS E-core throttle),release build, the
ourios-benchcompactionband-scale one-shot at theexact §9.7 shape (
FILES=32 ROWS=4800 BODY_BYTES=4096, ~453 MiB):baseline-8vcpu-32gib)Honest delta. These are not the same hardware (local M-series vs the
8 vCPU EPYC-Milan baseline), so ~138 vs 166.8 MiB/s is not a clean
sorting-overhead delta — it is a fresh indicative sorted measurement, plus
the observation that adding a full external-merge-sort pass lands in the same
order of magnitude as the unsorted copy-through and comfortably clears "keeps
up" (≫ any per-partition seal rate). The clean same-hardware sorted-vs-unsorted
delta is a
validatedbaseline-rerun item (RFC 0031 §3.2 opt-in), notasserted here. Recorded in
docs/benchmarks.md§9.25 as a new indicativerecord. The in-repo RFC0036.3 assertion is structural (D3 + memory) — no
flaky wall-clock gate (RFC 0036 §6). The criterion micro-sweep was confirmed to
still execute with the sort in the path.
Invariant / hazard note
Touches hazard #4 (small-file problem): this scenario pins the §3.2
memory property — clustering the compacted partition must not regress
compaction to whole-partition residency — and confirms the file band (D3) is
untouched by the smaller compacted row-group threshold. Per CLAUDE.md
§6.2, the memory bound is exercised through the real spill path via the
internal
SortTuningseam (a property, driven end-to-end), not asserted byinspection.
Verification (local, all green)
cargo fmt --all --check— exit 0cargo clippy --all-targets --all-features -- -D warnings— clean (workspace)cargo test --all-features— exit 0 (54/54 result lines, 0 failed; bothrfc0036_3 tests pass; rfc0036_5 still ignored + fail-on-todo when force-run)
cargo doc --workspace --no-deps --all-features— cleancargo bench -p ourios-bench --bench compaction— baseline one-shot + criterion sweep both runmdbook build— clean🤖 Generated with Claude Code
https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Summary by CodeRabbit
Bug Fixes
Documentation