feat(querier): resolve partition files through the RFC 0009 manifest - #96
Merged
Conversation
First implementation slice of RFC 0009 (compaction), sequenced
reader-first per the RFC §7 decision: the querier learns the
per-partition manifest before any compactor writes one.
- ourios-parquet: new `manifest` module — `Manifest { generation,
files }` + `Manifest::read` (Ok(None) when absent) + `to_json`,
the shared definition the future compactor will write. Colocated
unit tests (round-trip, absent→None, malformed→parse error).
- ourios-querier: `resolve_live_files` walks the tenant partition
tree and, per partition, takes the manifest's named files when a
`manifest.json` is present (authoritative — orphans/`.tmp`
ignored) else falls back to globbing `*.parquet`. The query is
built over that explicit file set via
`ListingTableConfig::new_with_multi_paths` + `infer_schema`
(which still merges schemas, preserving RFC0007.4 forward-compat).
Table partition columns are no longer declared — the data files
don't carry year/month/day/hour and the query filters only data
columns — which also simplifies the read path.
A manifest with no entry for a file makes that file invisible to
queries, so a committed compaction's superseded inputs are never
double-counted (read-half of RFC0009.3). New integration tests
(tests/manifest.rs) prove the manifest restricts to its named files
and that a manifest naming a consolidated file avoids the
double-count. All existing querier tests (B1 pruning, forward-compat,
tenant isolation, spaced paths, IO-error propagation) still pass.
The compactor that writes manifests is a later slice (epic #94).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a per-partition manifest model in ourios-parquet and updates ourios-querier to resolve live parquet inputs using per-partition ChangesReader-side per-partition manifest support
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the reader-side of RFC 0009’s per-partition manifest: the querier now resolves the authoritative live Parquet files via manifest.json (when present) and falls back to *.parquet discovery when absent, enabling compaction without query double-counting.
Changes:
- Added
ourios-parquet::manifestwithManifest { generation, files },Manifest::read(absent →Ok(None)), andto_json, plus unit tests. - Updated
ourios-querierto build DataFusion listing tables from an explicit per-file path set derived from manifests (or glob fallback), and added integration tests covering the RFC 0009 scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/ourios-querier/src/lib.rs | Resolves live files via per-partition manifests (or glob fallback) and builds the query table from explicit file paths. |
| crates/ourios-querier/tests/manifest.rs | Adds integration tests demonstrating manifest authority and prevention of compaction double-counting. |
| crates/ourios-parquet/src/manifest.rs | Introduces the shared Manifest JSON format + read/serialize helpers and unit tests. |
| crates/ourios-parquet/src/lib.rs | Exposes the new manifest module and types via public re-exports. |
💡 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: 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/manifest.rs`:
- Around line 76-84: The Manifest::read function currently accepts arbitrary
strings for Manifest.files; after deserializing in read(), iterate over
manifest.files and validate each entry is a plain partition-local filename (not
absolute, contains exactly one normal path component, and has no RootDir or
ParentDir components and no path separators). If any entry fails, return an
error (e.g., ManifestError::Parse or a new ManifestError::InvalidEntry) instead
of returning the manifest; update read() to perform this validation right after
serde_json::from_slice and reference Manifest, files, read(), MANIFEST_FILENAME,
and ManifestError when making the change.
In `@crates/ourios-querier/src/lib.rs`:
- Around line 146-201: Add colocated unit tests in the same module as
resolve_live_files (e.g., inside lib.rs under a #[cfg(test)] mod tests) that
exercise the manifest/glob resolution edge cases: create temporary directory
layouts and assert behavior for (1) missing tenant directory returns Ok(empty
Vec) when path doesn't exist, (2) malformed manifest.json produces a
QueryError::Storage (exercise Manifest::read failure), (3) partitions containing
only *.parquet.tmp yield no files, and (4) manifests are authoritative
(manifest.files override on-disk .parquet files). Use tempdir/tempfile to create
files and directories, call resolve_live_files, and assert results or error
kinds accordingly, referencing resolve_live_files and Manifest::read in the
tests.
🪄 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: 2b1f519c-a3fc-47d8-a6b3-929e3ca1d8be
📒 Files selected for processing (4)
crates/ourios-parquet/src/lib.rscrates/ourios-parquet/src/manifest.rscrates/ourios-querier/src/lib.rscrates/ourios-querier/tests/manifest.rs
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jun 3, 2026
jensholdgaard
added a commit
that referenced
this pull request
Jun 3, 2026
* feat(parquet): add sealed-partition compaction module (RFC 0009) 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> * fixup! feat(parquet): add sealed-partition compaction module (RFC 0009) * fixup! fixup! feat(parquet): add sealed-partition compaction module (RFC 0009) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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
First implementation slice of RFC 0009 (background compaction), sequenced reader-first per the RFC §7 decision: the querier learns the per-partition manifest before any compactor writes one, so there's no flag day. (RFC 0009 is
specified, #95; epic #94.)Changes
ourios-parquet— newmanifestmodule.Manifest { generation, files }+Manifest::read(Ok(None)when absent) +to_json— the shared definition the future compactor will write and the querier reads. Colocated unit tests (JSON round-trip, absent→None, malformed→parse error).ourios-querier— manifest-aware file resolution.resolve_live_fileswalks the tenant partition tree; per partition, it takes the manifest's named files when amanifest.jsonis present (authoritative — orphaned/.tmpfiles ignored), else falls back to globbing*.parquet(every partition today). The query is then built over that explicit file set viaListingTableConfig::new_with_multi_paths+infer_schema(which still merges schemas, preserving RFC0007.4 forward-compat). Table partition columns are no longer declared — the data files don't carryyear/month/day/hourand the query filters only data columns — which also simplifies the read path.Why (RFC0009.3 read-half)
A manifest with no entry for a file makes that file invisible to queries, so a committed compaction's superseded inputs are never double-counted. The query reads one consistent generation.
Tests (
tests/manifest.rs)rfc0009_3_manifest_restricts_to_named_files— two files in one partition + a manifest naming one → query sees only the named file's rows (4 → 2).rfc0009_3_manifest_naming_compacted_file_avoids_double_count— inputs and a consolidated file on disk; without a manifest the glob double-counts (8), with a manifest naming only the compacted file each row counts once (4), and template-exact pushdown still works.Invariants / hazards
infer_schemaover the multi-path set still merges heterogeneous schemas (its live test passes).data/tenant_id=<enc>/dir.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✅ (manifest unit + integration + all prior querier tests)The compactor that writes manifests (atomic generation swap + GC) is the next slice (epic #94). Epic: #94.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests