CTTY commented on code in PR #1735:
URL: https://github.com/apache/iceberg-rust/pull/1735#discussion_r2437282079
##########
crates/iceberg/src/writer/mod.rs:
##########
@@ -260,8 +270,8 @@ pub trait IcebergWriterBuilder<I = DefaultInput, O =
DefaultOutput>:
{
/// The associated writer type.
type R: IcebergWriter<I, O>;
- /// Build the iceberg writer.
- async fn build(self) -> Result<Self::R>;
+ /// Build the iceberg writer for an optional partition key.
+ async fn build_with_partition(self, partition_key: Option<PartitionKey>)
-> Result<Self::R>;
Review Comment:
Sorry I thought I replied earlier but it seems my comment didn't go thru.
I think your advice makes sense but it would be better to fix this in a
separate PR.
Changing `IcebergWriterBuilder::builder` to `&self` means we will need to
change all the writer builder along the writers chain to use `&self`
(IcebergWriterBuilder -> RollingFileWriterBuilder -> FileWriterBuilder).
Otherwise, the builder would look like the following and won't help too much
since we still need to clone the inner. This also can cause further confusion
to users since we have different semantics for each writer builder
```rust
async fn build(&self, partition_key: Option<PartitionKey>) ->
Result<Self::R> {
Ok(DataFileWriter {
inner: Some(self.inner.clone().build()), // inner builder still
needs to be cloned
partition_key,
})
}
```
Please let me know your thoughts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
