-
Notifications
You must be signed in to change notification settings - Fork 140
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
feat: Add equality delete writer #372
base: main
Are you sure you want to change the base?
feat: Add equality delete writer #372
Conversation
crates/iceberg/src/writer/base_writer/equality_delete_writer.rs
Outdated
Show resolved
Hide resolved
impl<B: FileWriterBuilder> IcebergWriter for EqualityDeleteFileWriter<B> { | ||
async fn write(&mut self, batch: RecordBatch) -> Result<()> { | ||
let batch = self.project_record_batch_columns(batch)?; | ||
self.inner_writer.as_mut().unwrap().write(&batch).await |
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.
We should use ?
rather than unwrap()
here.
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.
?
is for Result
but self.inner_writer
is an Option
. I think it is reasonable to leave it just Option
. Maybe self.inner_writer.as_mut().ok_or_else(...).write(&batch).await
is better?
crates/iceberg/src/writer/base_writer/equality_delete_writer.rs
Outdated
Show resolved
Hide resolved
crates/iceberg/src/writer/base_writer/equality_delete_writer.rs
Outdated
Show resolved
Hide resolved
crates/iceberg/src/writer/base_writer/equality_delete_writer.rs
Outdated
Show resolved
Hide resolved
2aa2e5f
to
f4c6cfa
Compare
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.
} else { | ||
Err(Error::new( | ||
ErrorKind::Unexpected, | ||
"Equality delete inner writer does not exist", |
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 think returning an unexpected error here is reasonable. I will fix
/// After close, regardless of success or failure, the writer should never be used again, otherwise the writer will panic. |
0cc7049
to
d140194
Compare
Close #341
EqualityDeleteWriter
refers tohttps://iceberg.apache.org/spec/#equality-delete-filesThis pr complete the implementation of
EqualityDeleteWriter
and its test case