refactor(parquet): shared BufferedParquetWriter core for data + audit writers - #758
Conversation
… writers The data writer and the audit writer were lifecycle twins: buffer rows into an in-memory ArrowWriter in sub-batches with a §3.5 flush threshold, poison on Parquet failure, publish via one store `put` on close. The twin halves (`append_chunks`, `close`, the object-key construction, the local-store scaffolding, the poisoning docs) drifted independently. New `parquet_io.rs` owns that lifecycle once: - `WriteError` ctor trait (mirrors decode.rs's `DecodeError`) so one core serves both public error enums. - `write_chunked` — the shared sub-batch loop, also backing the one-shot `encode_records_to_parquet`. Its `ChunkError` splits failures by origin, making the poison-on-Parquet-only rule structural instead of a `matches!` at every caller. - `BufferedParquetWriter` — inner/store/key/final_path/num_rows/ flush_bytes/poisoned + open/append/close; `Writer` and `AuditWriter` keep what actually differs (schema, props, encoder, partition pre-check, result type). Behavior is unchanged: same key shapes, same error variants, same poisoning and atomicity contracts, same flush thresholds (data 1024 rows / adaptive bytes, audit 256 rows / 128 MiB). Four colocated tests pin the core's invariants (key shape, publish + counts, encode errors don't poison, Parquet errors poison append and close). Verified: fmt, clippy -D warnings, ourios-parquet 180/180 (176 baseline + 4 new), compaction/rfc0009/rfc0005/rfc0042_8 filter 67/67, ourios-ingester suite green. Wave 2 of #745 (parquet slice A, writer half; decode half was #757). Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
|
Warning Review limit reachedNext included review available in 38 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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.
🟢 Approval recommended
The refactor centralizes previously duplicated write/poison/publish logic and includes targeted tests to pin the invariants, with only an optional maintainability follow-up noted.
Pull request overview
This PR continues the Parquet refactor epic (#745, wave 2) by extracting the shared “buffer → chunked write/flush → poison-on-Parquet-error → publish-on-close” lifecycle into a single private core (parquet_io.rs) and wiring both the data writer (Writer) and audit writer (AuditWriter) through it, keeping their public error enums and external behavior stable.
Changes:
- Introduces
parquet_io.rswithWriteError,ChunkError/write_chunked, andBufferedParquetWriteras the shared write-side core (including colocated invariant tests). - Refactors
WriterandAuditWriterto delegate buffer management, poisoning, and publish-on-close toBufferedParquetWriter. - Updates the one-shot
encode_records_to_parquet_with_promotedpath to use the sharedwrite_chunkedloop for the §3.5 row-group sizing rule.
File summaries
| File | Description |
|---|---|
| crates/ourios-parquet/src/writer.rs | Switches data writer to BufferedParquetWriter; reuses shared object-key, local-store open, and chunked write loop. |
| crates/ourios-parquet/src/audit_writer.rs | Switches audit writer to BufferedParquetWriter; reuses shared object-key and local-store open helper. |
| crates/ourios-parquet/src/parquet_io.rs | Adds the shared buffer-and-put core + chunked write helper + poisoning logic + new tests. |
| crates/ourios-parquet/src/lib.rs | Registers the new private parquet_io module. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review feedback on the writer core: parquet_io.rs hosted the WriteError impls for both writers' error enums, so the shared core depended back on the concrete writers. Each enum now implements the trait next to its own definition (writer.rs / audit_writer.rs), and the same move is applied to decode.rs's DecodeError impls (reader.rs / audit_reader.rs) so the two shared modules follow one rule: the core defines the trait, each consumer plugs in without editing the core. Pure code motion — no behavior change; parquet 180/180 unchanged. Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
There was a problem hiding this comment.
🟢 Approval recommended
The refactor cleanly centralizes the shared writer invariants (flush sizing + poison/publish contract) behind a private core with targeted tests, and I did not find any behavioral regressions in the updated call paths.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The refactor keeps the write-side invariants centralized with targeted tests and preserves existing public error/behavior contracts in the reviewed diffs.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The refactor is structurally contained, preserves existing contracts, and adds colocated tests that pin the shared write-core poisoning and publish semantics.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Wave 2 of the refactor epic #745 — the writer half of parquet slice A (the decode half landed as #757).
What
WriterandAuditWriterwere lifecycle twins that drifted independently. This PR extracts the shared buffer-and-put lifecycle into a new privateparquet_io.rs:WriteErrorctor trait (mirroringdecode.rs'sDecodeErrorfrom refactor(parquet): one accessor family decodes for both readers #757): the core stays generic while each writer's public error enum stays intact.write_chunked: the one sub-batch encode-and-write loop with the §3.5 flush-before-every-sub-batch rule, also backing the one-shotencode_records_to_parquet. ItsChunkErrorenum splits failures by origin, so poison on Parquet errors only is now structural rather than amatches!repeated at each caller.BufferedParquetWriter: ownsinner/store/key/final_path/num_rows/flush_bytes/poisonedwithopen/append/close(+object_keyandopen_local_storehelpers).Writerkeeps partition identity, promoted set, schema/props, and the §3.9 partition pre-check;AuditWriterkeeps its audit-axis pre-check and result type.Net: −352 twin lines; the poisoning/atomicity doc contract now lives in one place.
Behavior
Unchanged. Same object-key shapes, same error variants (including
AuditWriterError::Io.source_path: None), same poisoning + atomic-publish contracts, same thresholds (data: 1024-row sub-batches,ROW_GROUP_FLUSH_BYTES/adaptive; audit: 256-row sub-batches,ROW_GROUP_FLUSH_BYTES). One deliberate ordering note: bothappend_*methods still check poisoned before partition validation, preserving the existing fail-fast contract.Invariants touched (CLAUDE.md §4, hazard #4 / RFC 0005 §3.5)
The row-group sizing rule and the poison-refuses-to-publish rule move but do not change; both are now pinned by colocated tests in
parquet_io.rs:object_key_is_slash_delimited_with_parquet_suffixclose_publishes_bytes_and_reports_countsencode_error_does_not_poisonparquet_error_poisons_appends_and_close(forces a realArrowWriter::writefailure via a schema-mismatched batch)Gates run locally
cargo fmt --all --check,cargo clippy --all-targets --all-features -- -D warnings(parquet crate): cleanourios-parquet: 180/180 (176 baseline conserved + 4 new core tests)ourios-ingestersuite: all green🤖 Generated with Claude Code
https://claude.ai/code/session_01JZXtbyWoQY19ZGtNecDfgv