feat(parquet): migrate the writer to buffer-and-put on the Store seam (rfc0013 green) - #236
Conversation
… (rfc0013 green) `Writer` now encodes into an in-memory `ArrowWriter<Vec<u8>>` and publishes the finished bytes via `Store::put_blocking` on close, instead of streaming to a `<uuid>.parquet.tmp` File and renaming it. The object-store put is the atomic commit point (the local backend stages + renames internally), preserving the "readers see a complete file or nothing" guarantee. Atomicity and durability are unchanged from the prior scheme (object_store's local put does staged + rename + no-fsync, exactly as `close` did); only the on-disk temp artifact is gone. Row-group sizing (§3.5) still applies within the buffer; `num_rows` is now tracked directly (incremented per accepted sub-batch) since `into_inner` returns the buffer, not metadata. CONTRACT CHANGE (§6.2, maintainer pre-approved to land with the S3 work): the `.parquet.tmp`-during-write / Drop-cleanup behaviour is gone. `tests/partition_ layout.rs`'s atomic-publish test is rewritten to the buffer-and-put contract — nothing is published before `close`, a writer dropped without `close` publishes nothing (no temp artifact to clean), and `close` publishes the final `<uuid>.parquet` atomically. The `Drop` impl is removed (no temp file to remove) and `WriterError::Io`'s now-unused `source_path` (two-path/rename) field is dropped. The data `Writer` is the only migration here; the audit writer keeps its temp-file scheme (separate, out of this slice). Memory note: buffer-and-put holds the whole file in memory before the put (the §4 256MB–2GB target-file tradeoff inherent to the chosen model); `put_multipart` streaming is a future optimisation. Compaction (the writer's main consumer) passes unchanged; full workspace green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 6 minutes and 8 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)
✨ 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
This PR completes the RFC 0013 “write half” seam for ourios-parquet by migrating the partition data Writer from a temp-file + rename workflow to an in-memory buffer that is atomically committed via Store::put_blocking on close, and updates the partition layout integration test to match the new publish contract.
Changes:
- Refactors
Writerto encode Parquet intoArrowWriter<Vec<u8>>and publish bytes throughStore::put_blockingonly onWriter::close. - Tracks
num_rowsincrementally duringappend_records(sinceinto_innerreturns bytes, not Parquet metadata). - Updates
partition_layouttest coverage to assert “publish only on close; drop publishes nothing; no.parquet.tmpartifact”.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/ourios-parquet/src/writer.rs | Switches writer to buffer-and-put via Store, removes temp-file lifecycle, and adds direct row counting. |
| crates/ourios-parquet/tests/partition_layout.rs | Updates atomic-publish contract test to validate the new buffer-and-put behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
RFC 0013 green — completes the
.2seam for the write half:Writernow publishes throughStoreinstead of a temp file + rename. Companion to the reader (#233) and manifest (#234) seams.What
Writerencodes into an in-memoryArrowWriter<Vec<u8>>andStore::put_blockings the finished bytes onclose(was: stream to<uuid>.parquet.tmpFile, rename on close).putis the atomic commit point — the local backend stages to a private temp object and renames internally, so readers still see a complete file or nothing. Atomicity + durability are unchanged (object_store local put = staged + rename + no-fsync, exactly as the oldclosedid per its own docs); only the on-disk temp artifact is gone.num_rowstracked directly (per accepted sub-batch) sinceinto_innerreturns the buffer, not metadata. Row-group sizing (§3.5) still applies within the buffer.Dropimpl (no temp file to clean) andWriterError::Io's now-unusedsource_pathfield; tidied the temp/rename docs.The
.parquet.tmp-during-write and Drop-cleanup behaviour is gone.tests/partition_layout.rs's atomic-publish test is rewritten to the buffer-and-put contract:close;closepublishes nothing (no temp artifact to clean up);closepublishes the final<uuid>.parquetatomically, no.tmpleft behind.The test doc spells out the before/after. Only the data
Writermigrates here; the audit writer keeps its temp-file scheme (separate concern, out of scope).Invariants / notes
put_multipartstreaming is a future optimisation.cargo testgreen; clippy-D warnings+cargo doc -D warningsclean.🤖 Generated with Claude Code