feat(parquet): add sealed-partition compaction module (RFC 0009) - #97
Merged
Conversation
Next compaction slice (epic #94), building on the reader-side manifest support (#96): the compactor that consolidates a partition's small files and commits via the manifest. - `Manifest::write_atomic` — serialize to `manifest.json.tmp`, then rename over `manifest.json`. The rename is the commit point (RFC 0009 §3.4); a reader sees the old or new manifest, never a partial write. Validates entries before any bytes hit disk. - new `compaction` module — `compact_partition(bucket_root, &PartitionKey)` reads a partition's live files (manifest or glob), rewrites their rows as one file via Reader/Writer (rows copied, never re-mined — RFC0009.2), commits the manifest to name only the consolidated file, then GCs the superseded inputs. `Reader:: open_partition` validates every row belongs to the partition (§3.9 / RFC0009.5). A no-op for <2 live files. Correctness: on a partition with no prior manifest, the compactor first bootstraps a manifest naming the current inputs (the same set the glob returns — no visible change), making the reader manifest-authoritative before the consolidated file appears, so a concurrent query never sees inputs + new together (RFC0009.3, no torn read). Any failure before the commit leaves the inputs untouched; a crash after the commit leaves only harmless orphans the manifest already excludes. Colocated tests: row-preserving consolidation (2 files → 1, all rows intact), single-file no-op, generation bump from an existing manifest, plus manifest `write_atomic` round-trip / invalid-entry rejection. The background scheduler (sealed-partition selection, cadence) and orphan-GC sweep are later slices. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds atomic manifest publishing and sealed-partition compaction: ChangesPartition Compaction and Manifest Safety
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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.
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-parquet/src/compaction.rs`:
- Around line 148-153: The current compaction code collects every row into a
single Vec (the local variable rows) by calling
Reader::open_partition(...).read_all(), which can OOM; instead stream each input
file into the Writer: for each file in inputs call Reader::open_partition(file,
partition.clone()), iterate over its records (do not call read_all or extend
into rows), call Writer::write_row (or the appropriate write method) for each
record and increment the running row count; after finishing a file continue with
the next—apply the same change for the similar block that currently
reads/extends rows (the 158-163 section) so no full-partition buffering occurs.
- Around line 180-194: The loop that GC's superseded input files after commit
currently returns Err on any remove_file failure (in compaction.rs where
std::fs::remove_file(file) is matched), causing the whole compaction to be
reported as failed; instead, swallow non-critical delete errors: do not return
CompactionError::Io after commit—log or warn about the failing path (use the
existing logging/tracing facility) and continue to the next file so cleanup
failures do not mark a committed compaction as failed. Locate the for file in
&inputs loop and replace the Err(source) branch that returns Err with a
non-fatal log (including op="remove superseded input", the path and source) and
continue.
🪄 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: d356c389-f0a3-4124-8036-8384053b6c32
📒 Files selected for processing (3)
crates/ourios-parquet/src/compaction.rscrates/ourios-parquet/src/lib.rscrates/ourios-parquet/src/manifest.rs
Contributor
There was a problem hiding this comment.
Pull request overview
Adds the writer-side half of RFC 0009’s sealed-partition compaction flow in ourios-parquet: an atomic manifest swap as the commit point, plus a compactor that rewrites a partition’s live Parquet inputs into a single consolidated file and then GC’s superseded inputs.
Changes:
- Add
Manifest::write_atomicto publishmanifest.jsonvia amanifest.json.tmp+ rename swap, plus unit tests. - Introduce
compactionmodule withcompact_partition(bucket_root, &PartitionKey)implementing bootstrap-manifest → read → rewrite → commit → GC, plus colocated tests. - Export the new module and public compaction API types from
ourios-parquet’slib.rs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/ourios-parquet/src/manifest.rs | Adds atomic manifest rewrite (write_atomic) and tests verifying overwrite + invalid-entry rejection. |
| crates/ourios-parquet/src/lib.rs | Wires the new compaction module into the crate’s public API (module + re-exports). |
| crates/ourios-parquet/src/compaction.rs | Implements sealed-partition compaction and tests (two→one compaction, no-op behavior, generation bump). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ourios-parquet/src/compaction.rs (1)
133-133: 💤 Low valueConsider caching the manifest read to avoid a second I/O round-trip.
live_files()already reads the manifest at line 232, but the result (and generation) is discarded. ThenManifest::read()is called again at line 152 solely to retrieve the generation. Under the single-writer assumption this is correct, but it's wasteful I/O.A possible refactor: have
live_filesreturn(Vec<PathBuf>, Option<Manifest>)or introduce a helper struct, so the generation is available without a second read.Also applies to: 151-154
🤖 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/src/compaction.rs` at line 133, The code is doing duplicate manifest I/O: live_files(&partition_dir) already reads the manifest but its Manifest is discarded and Manifest::read(...) is called again to get generation; refactor live_files to return both the list of live file paths and the optional Manifest (e.g., -> (Vec<PathBuf>, Option<Manifest>)) or a small helper struct, then update callers (the site assigning let inputs = live_files(&partition_dir)? and the code that currently calls Manifest::read(...) for generation) to use the manifest returned from live_files to obtain the generation instead of performing a second Manifest::read; ensure all call sites handle None/manifests consistently and remove the redundant Manifest::read invocation.
🤖 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/src/compaction.rs`:
- Line 133: The code is doing duplicate manifest I/O: live_files(&partition_dir)
already reads the manifest but its Manifest is discarded and Manifest::read(...)
is called again to get generation; refactor live_files to return both the list
of live file paths and the optional Manifest (e.g., -> (Vec<PathBuf>,
Option<Manifest>)) or a small helper struct, then update callers (the site
assigning let inputs = live_files(&partition_dir)? and the code that currently
calls Manifest::read(...) for generation) to use the manifest returned from
live_files to obtain the generation instead of performing a second
Manifest::read; ensure all call sites handle None/manifests consistently and
remove the redundant Manifest::read invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 44d63700-aafa-4779-8df5-560a1ac26115
📒 Files selected for processing (2)
crates/ourios-parquet/src/compaction.rscrates/ourios-parquet/src/manifest.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/ourios-parquet/src/manifest.rs
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jun 3, 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.
What
Next compaction slice (epic #94), building on the reader-side manifest support (#96): the compactor that consolidates a partition's small files and commits via the manifest. RFC 0009 §3.4/§3.5.
Changes
Manifest::write_atomic— serialize tomanifest.json.tmp, thenrenameovermanifest.json. The rename is the commit point (RFC 0009 §3.4): a reader sees the old or new manifest, never a partial write. Entries are validated before any bytes hit disk.compactionmodule —compact_partition(bucket_root, &PartitionKey):Reader::open_partition(which validates every row's tenant + time bucket against the partition — §3.9 / RFC0009.5),Writer(rows copied, never re-mined — RFC0009.2),A no-op for fewer than two live files.
Correctness (RFC0009.2 / .3 / .5)
open_partitionaborts on a mis-partitioned input rather than merging it.Tests (colocated)
compacts_two_files_into_one_preserving_rows— 2 files (5 rows) → 1 live file, all rows intact, inputs GC'd.single_file_partition_is_a_no_op.bumps_generation_from_an_existing_manifest(5 → 6).Manifest::write_atomicround-trip / overwrite / invalid-entry rejection.Not in scope (later epic #94 slices)
The background scheduler (sealed-partition selection, cadence/grace — RFC 0004), the standalone orphan-GC sweep, audit events + telemetry, and the crash-recovery/proptest + D2/D3 bench.
Verification (local)
cargo fmt --all --check✅cargo clippy -p ourios-parquet -p ourios-querier --all-targets --all-features -- -D warnings✅cargo test -p ourios-parquet -p ourios-querier --all-features✅Epic: #94.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior
Tests