feat(parquet): rfc0013 green — buffer-and-put encode/decode + store round-trip - #232
Conversation
|
Warning Review limit reached
More reviews will be available in 49 minutes and 35 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 ChangesIn-memory Parquet encode, reader validation, and round-trip test
Sequence Diagram(s)sequenceDiagram
participant Test
participant encode_records_to_parquet
participant Store
participant Reader
rect rgba(70, 130, 180, 0.5)
note over Test,encode_records_to_parquet: Encode
Test->>encode_records_to_parquet: records[0..500], DEFAULT_ZSTD_LEVEL
encode_records_to_parquet-->>Test: Vec<u8> (Parquet bytes)
end
rect rgba(60, 179, 113, 0.5)
note over Test,Store: Store round-trip
Test->>Store: put(key, bytes)
Test->>Store: get(key)
Store-->>Test: Bytes
end
rect rgba(210, 105, 30, 0.5)
note over Test,Reader: Decode & assert
Test->>Reader: open_bytes(bytes)
Reader-->>Test: decoded records
Test->>Test: assert row count == 500 and records match
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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.
Pull request overview
Implements RFC 0013 “buffer-and-put” primitives in ourios-parquet: encoding MinedRecords into in-memory Parquet bytes for object-store PUT, and decoding from in-memory bytes for object-store GET, plus a local-backend round-trip test. This is a foundation step toward migrating ingester/querier I/O off filesystem paths and onto Store.
Changes:
- Add
encode_records_to_parquet(&[MinedRecord], zstd_level) -> Vec<u8>for in-memory Parquet encoding. - Add
Reader::open_bytes(bytes::Bytes)and factor the RFC 0005 §3.9 baseline REQUIRED-column validation into a shared helper. - Add an end-to-end buffer-and-put round-trip test using
Store::local, plus a directbytesdependency.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-parquet/tests/buffer_and_put.rs | Adds an async round-trip test: encode → Store.put → Store.get → Reader::open_bytes → decode. |
| crates/ourios-parquet/src/writer.rs | Adds in-memory Parquet encoding helper encode_records_to_parquet. |
| crates/ourios-parquet/src/reader.rs | Adds open_bytes and factors baseline REQUIRED-column enforcement into require_baseline_columns. |
| crates/ourios-parquet/src/lib.rs | Re-exports encode_records_to_parquet. |
| crates/ourios-parquet/Cargo.toml | Adds direct bytes dependency to support Reader::open_bytes. |
| Cargo.lock | Records the added direct dependency usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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/writer.rs`:
- Around line 606-637: Add a `#[cfg(test)]` module colocated with the
`encode_records_to_parquet` function in this file that includes both unit tests
and property-based tests. The unit tests should verify basic encoding behavior
(e.g., that records are correctly encoded to valid Parquet bytes and can be
decoded). Include at least one `proptest` case that verifies the row-group
sizing invariant mentioned in the comments — specifically that when in-progress
buffer size exceeds ROW_GROUP_FLUSH_BYTES, row groups are properly sealed and
flushed, preventing oversized row groups from being produced even with large
inputs. Additionally, add tests for edge cases like empty record batches and
verify round-trip invariants (encoding then decoding records preserves data
integrity).
🪄 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: 0059404e-af1a-4acf-be15-db355b3c6653
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/ourios-parquet/Cargo.tomlcrates/ourios-parquet/src/lib.rscrates/ourios-parquet/src/reader.rscrates/ourios-parquet/src/writer.rscrates/ourios-parquet/tests/buffer_and_put.rs
…t (coderabbit, §6.2)
What
RFC 0013 green slice 2: the buffer-and-put I/O primitives (the maintainer-chosen object-store-native model) — encode
MinedRecords to in-memory Parquet bytes, decode from bytes, and a round-trip throughStore. Foundation for migrating the writer/reader off filesystem paths; RFC staysreduntil the §5 stubs pass (consumer migration, later slices).encode_records_to_parquet(records, zstd_level) -> Vec<u8>(writer.rs) — same schema/codec/§3.6 policy as the fileWriter, but targets aVec<u8>viaArrowWriter<Vec<u8>>(row-group sizing still applies in-buffer).Reader::open_bytes(bytes::Bytes)(reader.rs) — decode from in-memory bytes (theStore.getread path); same RFC 0005 §3.9 baseline-column check asopen_file, factored into a sharedrequire_baseline_columnshelper.tests/buffer_and_put.rs— 500 records →encode→Store.put→Store.get→open_bytes→read_all, asserting full row recovery (local backend).bytesdirect dep (the parquet reader's in-memoryChunkReadersource; already in the arrow/parquet tree).Verified locally
cargo fmt,cargo clippy -p ourios-parquet --all-targets(clean),cargo test -p ourios-parquet(the new round-trip passes; full suite 70+ green; the 8 §5 stubs + the sizing stub stay ignored),cargo deny check(ok —bytesadds no new graph entries).Next green slices
encode_records_to_parquet+Store.put(the async edge) — write side onto object storage.Store.get+Reader::open_bytes→ RFC0013.2 / .8.awsfeature) + MinIOtestcontainers→ RFC0013.1/.3/.5/.7; WAL-stays-local wiring → RFC0013.6.Invariants
Adds the buffer-and-put primitives; the file-based
Writer/Readerare unchanged (theopen_filerefactor is behaviour-preserving — shared helper). No consumer migrated yet. Serves §3.6; WAL stays local (§3.4).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements