andygrove commented on PR #6773: URL: https://github.com/apache/datafusion-comet/pull/6773#issuecomment-6061249358
Why this copies iceberg-rust's `FanoutWriter`: The fix needs two things that `FanoutWriter` can't do. iceberg-rust's [`FanoutWriter`](https://github.com/apache/iceberg-rust/blob/af1da4c5bc86178c38c1c6db0578bdd9fb7021e3/crates/iceberg/src/writer/partitioning/fanout_writer.rs) keeps its per-partition writers in a private `HashMap` and exposes only `write(partition_key, batch)` and `close(self)`, which closes every partition's writer at once. When the pool refuses the reservation, this PR has to close one partition's writer while the task carries on, and has to know which partitions hold the most memory to choose them. Neither is possible from outside `FanoutWriter`. So [`FanoutFiles`](https://github.com/apache/datafusion-comet/blob/f517aeee8d8b6aecf35a3a24abdc1e87591df340/native/core/src/execution/operators/iceberg_write.rs#L1137-L1208) is a copy of `FanoutWriter`'s logic, about 70 lines with its doc comments: the same get-or-create writer per partition and the same close. It adds `close_partition`, which closes one partition's writer and keeps its data files for the task's output, and `held`, which reads what a partition's open file holds from the `OpenFileMemory` its files now report to. It implements the same `PartitioningWriter` trait, so the write and close paths in `InnerWriter` did not change. iceberg-rust `main` has the same `FanoutWriter` API as the revision Comet pins. The closest upstream issue is apache/iceberg-rust#1744, which asks for a `max_open_partitions` cap. A cap on how many partitions are open doesn't bound memory, since what each partition holds depends on how many rows it gets. If upstream adds a way to close one partition's writer, `FanoutFiles` can go back to being a `FanoutWriter`, and Comet can keep tracking each partition's memory on its own side. -- 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]
