feat(parquet): RFC0013 red — store module skeleton + §5 ignored stubs - #230
Conversation
|
Warning Review limit reached
More reviews will be available in 35 minutes and 2 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 (2)
📝 WalkthroughWalkthroughIntroduces the RFC 0013 "red" scaffold in ChangesRFC 0013 Red Scaffold
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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.
Pull request overview
Starts the RFC 0013 “red” stage by adding an ourios-parquet object-store seam (Store) plus ignored acceptance-scenario test stubs, and updates the RFC/roadmap status to reflect the new gate.
Changes:
- Adds
ourios_parquet::storewith aStorewrapper overArc<dyn ObjectStore>, a wiredlocal()constructor, and ans3()placeholder for upcoming green work. - Introduces RFC 0013 §5 scenario test stubs as
#[ignore]dtodo!()tests. - Promotes RFC 0013 status to
redin docs (RFC + roadmap) and makesobject_storea direct dependency ofourios-parquet.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/roadmap.md | Updates the roadmap entry to reflect RFC 0013 moving to red and landing the store skeleton + stubs. |
| docs/rfcs/0013-object-storage.md | Flips RFC status to red and records the crate-shape decision plus the new scaffold. |
| crates/ourios-parquet/tests/rfc0013_object_store.rs | Adds the eight RFC §5 scenario stubs as ignored tests (red-gate scaffolding). |
| crates/ourios-parquet/src/store.rs | Introduces the Store/S3Config/StoreError skeleton and a working local backend constructor. |
| crates/ourios-parquet/src/lib.rs | Exposes the new store module and re-exports Store/S3Config/StoreError. |
| crates/ourios-parquet/Cargo.toml | Adds object_store as a direct dependency for the new storage seam. |
| Cargo.lock | Records the new direct dependency linkage for ourios-parquet. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9b49641 to
0ce8270
Compare
…t panic (copilot)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/ourios-parquet/src/store.rs (1)
33-47: ⚡ Quick win
S3Config::default()produces an invalid configuration.The
#[derive(Default)]onS3Configgenerates a default with an emptybucketstring, which is semantically invalid. An S3 configuration without a bucket name cannot be used.Consider removing the
Defaultderive or implementingDefaultmanually to return a configuration that is either explicitly invalid (e.g., with a marker value) or documenting that the default is not usable as-is.♻️ Option 1: Remove Default derive
-#[derive(Debug, Clone, Default)] +#[derive(Debug, Clone)] #[non_exhaustive] pub struct S3Config {Alternatively, if
Defaultis needed for the test stub at line 63 intests/rfc0013_object_store.rs, document that the default is not a usable configuration:♻️ Option 2: Document the default as a placeholder
/// Configuration for the S3 / S3-compatible backend (RFC0013.7). Populated /// from RFC 0004 config at `green`; a placeholder here so the `red` /// constructor signature is stable. +/// +/// Note: The `Default` impl produces an invalid configuration (empty bucket). +/// It exists only for test scaffolding during the `red` stage. #[derive(Debug, Clone, Default)]🤖 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/store.rs` around lines 33 - 47, The S3Config struct derives Default, which produces a configuration with an empty bucket string that is semantically invalid and cannot be used. Either remove the #[derive(Default)] attribute from S3Config to prevent invalid default instances, or implement Default manually for S3Config with clear documentation that the default is a placeholder configuration not intended for actual use. If a manual Default implementation is chosen, ensure it returns a S3Config that makes the placeholder nature explicit (such as with a marker bucket name) or document this limitation prominently.
🤖 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 95-113: The `#[allow(clippy::needless_pass_by_value,
unused_variables)]` attribute on the s3 function lacks a required explanation
comment per coding guidelines. Add a `//` comment above or inline with this
attribute explaining why the lints are being suppressed. The explanation should
note that the cfg parameter and unused variables are acceptable in the current
red (stub) implementation because they will be used once the green
implementation is completed.
---
Nitpick comments:
In `@crates/ourios-parquet/src/store.rs`:
- Around line 33-47: The S3Config struct derives Default, which produces a
configuration with an empty bucket string that is semantically invalid and
cannot be used. Either remove the #[derive(Default)] attribute from S3Config to
prevent invalid default instances, or implement Default manually for S3Config
with clear documentation that the default is a placeholder configuration not
intended for actual use. If a manual Default implementation is chosen, ensure it
returns a S3Config that makes the placeholder nature explicit (such as with a
marker bucket name) or document this limitation prominently.
🪄 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: 511b01bf-6128-43f7-a84e-c2a2adb1930e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/ourios-parquet/Cargo.tomlcrates/ourios-parquet/src/lib.rscrates/ourios-parquet/src/store.rscrates/ourios-parquet/tests/rfc0013_object_store.rsdocs/rfcs/0013-object-storage.mddocs/roadmap.md
…refix, S3Config default)
What
Starts the
redstage for RFC 0013 (object-storage backend) — the first code of the shipping milestone. Lands the scaffold thegreenwork fills; flips RFC 0013specified → red.object_storeis now a direct dep ofourios-parquet(was transitive via DataFusion;0.13unifies with the in-tree version).crates/ourios-parquet/src/store.rs— aStoretype wrappingArc<dyn ObjectStore>+ a key prefix, withlocal()(real,LocalFileSystem-backed),s3()(stubbed —green),object_store()(for handing to DataFusion on the read path), andS3Config/StoreError. Re-exported fromlib.rs.tests/rfc0013_object_store.rs— the 8 §5 scenarios (RFC0013.1–.8) as#[ignore]d stubs thattodo!(). Ignored so CI stays green and this red scaffold merges;greenun-ignores + implements each.Decision resolved at red
Crate shape (RFC 0013 §3.7 / §7 first open question): a
storemodule inourios-parquet, not a new crate. The dep graph confirms it —ourios-querier,-ingester,-serveralready depend onourios-parquet, so the type is visible to every storage consumer without theCLAUDE.md§7 new-crate commitment. Extracting anourios-storecrate later (if a non-Parquet consumer appears) is mechanical.Verified locally
cargo fmt,cargo clippy -p ourios-parquet --all-targets(clean),cargo test -p ourios-parquet(the rfc0013 binary reports 8 ignored, 0 failed; the rest of the suite green),cargo deny check(advisories/bans/licenses/sources ok — the now-directobject_storeadds no new graph entries).greennext (separate PRs)S3 backend (
object_storeawsfeature) + conditional-PUT atomic publish (RFC0013.3/.4); migrate writer/reader/compaction/audit frombucket_root: &PathontoStore; un-#[ignore]the stubs. The remaining §7 questions (lease mechanism, conditional-PUT portability, multipart, creds, cache, migration) get decided there.Invariants
Adds a storage seam; no behaviour change yet (consumers still use the
&Pathpath). Serves §3.6 (object storage = truth); WAL stays local (§3.4). Thestoreskeleton'ss3()isunimplemented!()(red), not on any production path.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation