-
Notifications
You must be signed in to change notification settings - Fork 861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add close method to RecordBatchWriter trait #4228
Add close method to RecordBatchWriter trait #4228
Conversation
…, JSON, IPC and Parquet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if calling the method close would instead avoid so many collisions, and might better hint at its consuming nature (maybe??)
arrow-array/src/record_batch.rs
Outdated
@@ -47,6 +47,9 @@ pub trait RecordBatchReader: Iterator<Item = Result<RecordBatch, ArrowError>> { | |||
pub trait RecordBatchWriter { | |||
/// Write a single batch to the writer. | |||
fn write(&mut self, batch: &RecordBatch) -> Result<(), ArrowError>; | |||
|
|||
/// Write footer or termination data, then mark the writer as done. | |||
fn finish(self) -> Result<(), ArrowError>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn finish(self) -> Result<(), ArrowError>; | |
fn close(self) -> Result<(), ArrowError>; |
Perhaps, this might avoid the name collisions?
Co-authored-by: Raphael Taylor-Davies <[email protected]>
Should be good now! Thank you for your review. |
For those following along, RecordBatchWriter hasn't yet been released so not marking as a breaking change |
Which issue does this PR close?
This PR does not close any particular issue. It is an alternative implementation of #4221
Rationale for this change
In #4221, the
finish
method takes&mut self
whereas in this PR it takesself
.We are forced to use fully qualified method calls for
finish
where we want to callfinish
on the object itself becauseRecordBatchWriter
is in scope (because weuse arrow_array::*
). What's your opinion on importing only needed items fromarrow_array
to avoid this?What changes are included in this PR?
Add
finish
method toRecordBatchWriter
and implement it for CSV, JSON, IPC and Parquet writers.Are there any user-facing changes?
According to my analysis no breaking change is introduced in this PR.