feat(parquet): route the manifest through the object-store seam (rfc0013 green) by jensholdgaard · Pull Request #234 · jensholdgaard/ourios

jensholdgaard · 2026-06-16T00:00:24Z

RFC 0013 green slice 4 — wires the RFC 0009 per-partition manifest onto the Store seam (local backend), behavior-preserving. Companion to the reader seam (#233).

What

Manifest::read → Store::local(partition_dir).get_blocking_opt(MANIFEST_FILENAME), guarded by try_exists so an absent partition dir still maps to Ok(None) (the prior std::fs NotFound→None contract — Store::local canonicalises its root and would otherwise error on a missing dir).
Manifest::write_atomic → Store::put_blocking. object_store's local put stages to a private temp object then renames into place, so the atomic-commit-point + no-fsync semantics are identical to the prior manifest.json.tmp + rename. Last-writer-wins Overwrite, unchanged.
New Store primitives: get_blocking_opt (NotFound→None) + StoreError::is_not_found; put_if_absent/put_if_absent_blocking (create-if-absent, If-None-Match) — the local-testable half of RFC 0013 conditional PUT.

Deferred (by design)

The generation compare-and-swap (RFC0013.3 contention, .4 CAS half) lands with the S3 backend: object_store's LocalFileSystem put_opts returns NotImplemented for PutMode::Update, so CAS can't be implemented or tested on the local backend. S3 testing will use real S3-compatible storage (Hetzner), not MinIO/LocalStack.

Invariants / scope

No on-disk format or layout change; the manifest bytes and atomicity contract are unchanged (RFC 0009 §3.4). Tenant isolation (§3.7) unaffected — same per-partition file.
Every existing RFC 0009 manifest + compaction suite passes unchanged through the seam (full workspace cargo test green). New unit tests cover get_blocking_opt (missing→None) and put_if_absent (refuses to clobber).

🤖 Generated with Claude Code

Summary by CodeRabbit

Refactor
- Improved internal manifest storage and retrieval through a unified abstraction layer, enabling more robust file I/O operations.
- Enhanced error handling for missing manifest files and added support for atomic write operations to ensure data consistency.

…013 green) Wire the RFC 0009 per-partition manifest read/write onto the RFC 0013 `Store` seam (local backend), behaviour-preserving: - `Manifest::read` reads via `Store::local(partition_dir).get_blocking_opt`, guarded by a `try_exists` check so an absent partition directory still maps to `Ok(None)` (the prior `std::fs` NotFound→None contract; `Store::local` canonicalises its root and would otherwise error on a missing dir). - `Manifest::write_atomic` writes via `Store::put_blocking`. object_store's local put stages to a private temp object and renames it into place, so the atomic-commit-point and no-fsync semantics are unchanged from the prior tmp+rename — last-writer-wins `Overwrite`. New `Store` primitives: - `get_blocking_opt` (NotFound→None) + `StoreError::is_not_found`. - `put_if_absent` / `put_if_absent_blocking` (create-if-absent, If-None-Match) — the local-testable half of RFC 0013 conditional PUT. The generation compare-and-swap (RFC0013.3/.4) is deferred to the S3 backend: `LocalFileSystem` rejects `PutMode::Update`, so CAS can't be implemented or tested on the local backend. Every existing RFC 0009 manifest + compaction suite passes unchanged through the seam. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

coderabbitai · 2026-06-16T00:00:37Z

Warning

Review limit reached

@jensholdgaard, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 43 minutes and 7 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2acdc303-7006-4928-929b-6bbad9f555ce

📥 Commits

Reviewing files that changed from the base of the PR and between 6d40e06 and 1d0392b.

📒 Files selected for processing (2)

crates/ourios-parquet/Cargo.toml
crates/ourios-parquet/src/store.rs

📝 Walkthrough

Walkthrough

Extends Store with get_blocking_opt, put_if_absent (async), put_if_absent_blocking, and StoreError::is_not_found. Rewires Manifest::read and Manifest::write_atomic to use these new Store methods instead of direct std::fs calls, adding a store_io error-mapping helper and updating associated doc comments.

Changes

Manifest I/O migration to Store abstraction

Layer / File(s)	Summary
Store: conditional-put and optional-get APIs `crates/ourios-parquet/src/store.rs`	Adds `StoreError::is_not_found`, `Store::get_blocking_opt` (maps not-found to `Ok(None)`), `Store::put_if_absent` (async, create-if-absent via `PutMode::Create`), and `Store::put_if_absent_blocking`; updates imports for `PutMode`, `PutOptions`, `PutPayload`; adds unit tests for missing-object read and clobber-prevention.
Manifest read/write via Store seam `crates/ourios-parquet/src/manifest.rs`	Adds private `store_io` error-mapping helper; rewrites `Manifest::read` to use `Store::local(...).get_blocking_opt` with early-return for absent partition directories; rewrites `Manifest::write_atomic` to commit via `Store::local(...).put_blocking` instead of `.tmp`+rename; doc comments updated to reflect new commit semantics.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

jensholdgaard/ourios#231: Extends store.rs with foundational put/get/delete on the same Store abstraction that this PR builds upon with put_if_absent and get_blocking_opt.
jensholdgaard/ourios#230: Introduced the Store/StoreError seam in store.rs and the Manifest wiring in manifest.rs that this PR directly extends.
jensholdgaard/ourios#233: Modifies store.rs to add sync↔async blocking access patterns and StoreError handling that this PR's manifest migration depends on.

Poem

🐇 Hopping past the old std::fs trail,
No more .tmp files, no rename to unveil!
get_blocking_opt finds the manifest with care,
put_if_absent guards what's already there.
The Store now speaks, the rabbit hops along —
Clean seams and Ok(None) keep the warren strong! 🌿

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: routing the manifest through the Store abstraction (RFC0013 green), which is the primary focus of this pull request.
Description check	✅ Passed	The description provides comprehensive context including what changed, why (RFC references), new primitives added, deferred scope, and invariants preserved. However, the checklist is incomplete with no items marked.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch rfc0013-green-manifest-seam

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull request overview

Routes RFC 0009 per-partition manifest I/O through the RFC 0013 Store object-storage seam (local backend), keeping on-disk format and atomicity semantics behavior-preserving while preparing for future S3 support.

Changes:

Added StoreError::is_not_found, Store::get_blocking_opt, and conditional create primitives (put_if_absent{,_blocking}) using PutMode::Create.
Updated Manifest::{read,write_atomic} to use Store::local(...).get_blocking_opt(...) / put_blocking(...), with a try_exists guard to preserve the prior NotFound→Ok(None) behavior for missing partition dirs.
Added unit tests for get_blocking_opt missing→None mapping and put_if_absent refusing to clobber.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
crates/ourios-parquet/src/store.rs	Extends the storage seam with not-found detection, optional-get, and create-if-absent put + tests.
crates/ourios-parquet/src/manifest.rs	Re-routes manifest reads/writes through `Store` while preserving the prior absence semantics and atomic overwrite behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The sync→async bridge built a fresh tokio runtime on every *_blocking call. Manifest::read goes through it once per partition, and the querier's resolve_live_files (on its async task) walks every partition per query — so a multi-partition query paid a runtime build per partition on the hot path. Build the runtime once via OnceLock::get_or_init (multi-thread, 1 worker) and reuse it. get_or_init can't drop a surplus runtime on a caller thread the way the earlier manual get/set could (that flaked with "drop a runtime in async context" when the loser was inside a #[tokio::test]); the runtime lives for the process and is never dropped. The build Result is cached so failure still surfaces as StoreError::Runtime without an expect. The per-call scoped thread stays — it's required so block_on runs off the caller's tokio context — but it no longer also builds a runtime. rt-multi-thread is back (a shared runtime is driven by concurrent block_on from many bridge threads; a current-thread runtime can't). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

jensholdgaard requested a review from Copilot June 16, 2026 00:01

Copilot started reviewing on behalf of jensholdgaard June 16, 2026 00:01 View session

Copilot AI reviewed Jun 16, 2026

View reviewed changes

Comment thread crates/ourios-parquet/src/manifest.rs

jensholdgaard requested a review from Copilot June 16, 2026 00:18

Copilot started reviewing on behalf of jensholdgaard June 16, 2026 00:18 View session

Copilot AI reviewed Jun 16, 2026

View reviewed changes

jensholdgaard merged commit 30a6efc into main Jun 16, 2026
13 checks passed

jensholdgaard deleted the rfc0013-green-manifest-seam branch June 16, 2026 00:22

jensholdgaard mentioned this pull request Jun 16, 2026

feat(parquet): migrate the writer to buffer-and-put on the Store seam (rfc0013 green) #236

Merged

This was referenced Jun 16, 2026

feat(parquet): manifest compare-and-swap publish (rfc0013 green .3/.4) #239

Merged

docs(rfc): storage-backend selection RFC 0019 (specified) #287

Merged

coderabbitai Bot mentioned this pull request Jun 27, 2026

feat(parquet): Store sized listing + blocking delete for the RFC 0019 compactor migration #293

Merged

Navigation Menu

feat(parquet): route the manifest through the object-store seam (rfc0013 green) - #234

feat(parquet): route the manifest through the object-store seam (rfc0013 green)#234
jensholdgaard merged 2 commits into
mainfrom
rfc0013-green-manifest-seam

jensholdgaard commented Jun 16, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 16, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jensholdgaard commented Jun 16, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Deferred (by design)

Invariants / scope

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jensholdgaard commented Jun 16, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 16, 2026 •

edited

Loading