andygrove commented on PR #5473: URL: https://github.com/apache/datafusion-comet/pull/5473#issuecomment-5428062288
The `pub use RssPartitionWriter` doesn't actually expose anything usable yet. `PartitionWriter` is `pub(crate)`, so all three of `write`, `finish_partition`, and `finish_all` live on a trait that downstream crates can't import — a crate outside `datafusion-comet-shuffle` can call `try_new` and then do nothing with the result. I confirmed this with a throwaway integration test in `native/shuffle/tests/`: `error[E0599]: no method named 'write' found for mutable reference `&mut RssPartitionWriter``. Since the stated goal here is to expose the writer for integration in subsequent PRs, could we either make `PartitionWriter` `pub` in this PR, or drop the re-export from `lib.rs` and add it in the PR that actually wires it up? If the integration is going to live inside the shuffle crate anyway (e.g. `ShuffleWriterExec` gaining an RSS mode), the second option seems cleaner — the writer stays `pub(crate)` alongside `LocalPartitionWriter` and the API surface doesn't grow. -- 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]
