andygrove commented on PR #4779: URL: https://github.com/apache/datafusion-comet/pull/4779#issuecomment-4895363372
Thanks for this, the separation of partitioning from storage reads cleanly and the behavior looks preserved across all three modes. One design question on the `PartitionWriter` trait. The generic `I: Iterator` parameter on `write` and `finish_partition` makes the trait non-object-safe, which is why the partitioners all had to become generic over `T: PartitionWriter` and `external_shuffle` is pinned to the concrete `LocalPartitionWriter`. For the remote-backend goal that motivates this refactor, that means adding a second backend would monomorphize each partitioner per backend and `external_shuffle` would need to branch on backend type while constructing differently-typed partitioners. Did you consider taking the iterator as `&mut dyn Iterator<Item = datafusion::common::Result<RecordBatch>>` instead? That would make `PartitionWriter` object-safe, so the partitioners could hold a `Box<dyn PartitionWriter>` and `external_shuffle` could select the backend once and pass it into any partitioner. Curious how you are picturing the remote writer getting wired in from `external_shuffle`. _Disclosure: I used an LLM to help review this PR and draft this comment._ -- 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]
