CTTY commented on code in PR #1735:
URL: https://github.com/apache/iceberg-rust/pull/1735#discussion_r2418446991
##########
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:
This is a breaking change. I believe this is necessary because:
1. `IcebergWriter` is supposed to generate `DataFile` that always hold a
partition value according to [iceberg
spec](https://iceberg.apache.org/spec/#data-file-fields:~:text=102-,partition,-struct%3C...%3E).
2. The existing code store partition value in the builder directly, making
`builder.clone()` useless:
```rust
let builder = IcebergWriterBuilder::new(partition_A);
let writer_A = builder.build();
... // write to partition A
// done with partition A and now we need to write to partition B
// this is wrong because partition value A is still stored in the builder
let writer_B = builder.clone().build()
```
An alternative is to add a new method `clone_with_partition()` but that
would also be a breaking change and it's less clean compared to
`build_with_partition()`
--
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]
