feat(parquet): implement the S3 / S3-compatible Store backend (rfc0013 green) - #235
Conversation
…3 green) `Store::s3(S3Config)` now builds an `object_store::aws::AmazonS3` (behind the new `aws` feature) instead of returning a placeholder — AWS S3 or any S3-compatible endpoint (Hetzner, R2, …) via `S3Config::endpoint`, with `allow_http` inferred for plain-HTTP dev endpoints. Credentials come from the standard AWS credential chain (env / profile / IMDS); `S3Config` carries only non-secret addressing. The backend keeps object_store's default `S3ConditionalPut::ETagMatch` — the `If-Match` CAS the manifest generation-swap will use (RFC0013.3/.4). Construction is offline (creds/connectivity resolve on first request), so it's unit-testable without a live endpoint. Supporting changes: - `StoreError::Config` for pre-construction config errors (empty bucket); removed the now-dead `red` `Unimplemented` variant. - Bridge runtime gains `enable_all()` + tokio `net`/`time` features so it can drive the S3 HTTP client; `Store` derives `Debug`. - The `aws` feature expands the dep tree (+~570 Cargo.lock lines, http/TLS stack); `cargo deny check` passes (advisories/bans/licenses/sources ok). Live round-trip / endpoint-override tests (RFC0013.1/.7) land in the next slice against real S3-compatible storage (Hetzner); the §5 stubs stay `#[ignore]`d. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 44 minutes and 10 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 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 selected for processing (1)
📝 WalkthroughWalkthroughAdds real S3 backend support to ChangesS3 Backend for ourios-parquet Store
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
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/store.rs`:
- Around line 216-222: The bucket validation check calls trim().is_empty() to
validate, but the original untrimmed bucket string is then passed to
with_bucket_name on the AmazonS3Builder. This allows whitespace-padded bucket
names to pass validation and cause opaque S3 errors later. Trim the bucket
string first and assign it to a variable, then use this trimmed variable for
both the is_empty() validation check and when passing it to with_bucket_name to
ensure consistent, upfront validation.
🪄 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: 2b02b55c-7f28-4a22-934d-210c4e2561dd
⛔ 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/store.rs
There was a problem hiding this comment.
Pull request overview
Implements the RFC0013 “green” slice for ourios-parquet storage by replacing the S3 placeholder with a real object_store::aws::AmazonS3 backend, enabling the Parquet layer to target AWS S3 or S3-compatible endpoints through a unified Store API.
Changes:
- Implement
Store::s3(S3Config)usingAmazonS3Builder::from_env()with optional endpoint/region overrides and upfront config validation. - Update the sync→async bridge runtime to
enable_all()and expandtokiofeatures to support the S3 HTTP client. - Add unit tests for offline S3 store construction and empty-bucket config rejection; update deps (
object_storewithawsfeature).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/ourios-parquet/src/store.rs | Adds real S3 backend construction, a new StoreError::Config, updates runtime setup, and introduces S3 construction tests. |
| crates/ourios-parquet/Cargo.toml | Enables object_store’s aws feature and adds tokio net/time features required by the S3 HTTP stack. |
| Cargo.lock | Lockfile updates from enabling the S3/AWS dependency graph. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…+ test region (review)
RFC 0013 green — first S3-arc slice:
Store::s3now builds a realAmazonS3backend (was a placeholder). Infra-independent; live tests follow.What
Store::s3(S3Config)→object_store::aws::AmazonS3viaAmazonS3Builder(newawsfeature). Works against AWS S3 or any S3-compatible endpoint (Hetzner, R2, …) throughS3Config::endpoint, inferringallow_httpfor plain-HTTP dev endpoints.AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY, shared profile, or IMDS);S3Configcarries only non-secret addressing (bucket/endpoint/region/prefix). Explicit RFC 0004 cred wiring is a later refinement.S3ConditionalPut::ETagMatch— theIf-MatchCAS the manifest generation-swap needs (RFC0013.3/.4), confirmed available on the S3 backend.StoreError::Configfor pre-construction errors (empty bucket → rejected up front); removed the now-deadredUnimplementedvariant.enable_all()+ tokionet/timeso it can drive the S3 HTTP client;StorederivesDebug.Tested
StoreError::Config. (object_store builds the backend without contacting the endpoint — creds/connectivity resolve on first request.)cargo testgreen; clippy-D warnings+cargo doc -D warningsclean.Supply chain
The
awsfeature expands the dep tree (+~570Cargo.locklines — http/TLS stack).cargo deny checkpasses (advisories/bans/licenses/sources ok; only pre-existing duplicate-version warnings).Deferred (next slice)
Live round-trip (RFC0013.1) + endpoint-override (RFC0013.7) tests run against real S3-compatible storage (Hetzner). The §5 stubs stay
#[ignore]d until then; this slice does not change any §5 status.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests