timsaucer commented on issue #1612: URL: https://github.com/apache/datafusion-python/issues/1612#issuecomment-4892061910
Here's what I think the differences are for the implementation of the options listed above. ### Option 1 - Bundled feature build We have to decide which distributed systems we will support. I suspect we want both `ballista` and `datafusion-distributed`. I would *prefer* to support those that are officially within the DataFusion umbrella. I think DataDog has `datafusion-distributed` stable enough and in production that they are generally willing to donate it to DataFusion. When we have a new major release of DataFusion, we would now need both `ballista` and `datafusion-distributed` to make new releases with the upgraded major version before we could do a major release update to `datafusion-python`. I suspect this would mean that our Python bindings would lag even further behind the upstream repo. We have been trying to keep `main` in this repo up to date with `main` on DataFusion to try to lower the gap between releases, because this repo also holds up other downstream work from upgrades. My suspicion is that this will add a 2-4 week delay in major releases, based on the way I've seen these release chains work in other projects like geodatafusion, lance, and then rerun (my company's project). Simply the need to have those projects upgrade and go through their own release cycles will slow down releases, not to mention trying to keep up to date with feature upgrades in the datafusion core repo. **For me, this is the biggest down side to this option.** We now need `datafusion-python` to add wrapper classes for anything that needs exposure in both `ballista` and `datafusion-distributed`. Since we already have https://github.com/apache/datafusion-python/pull/1611 we have a very good measure of what the burden is, and a jump start on supporting it. I expect `ballista` to have similar level of effort. We would then need to update our skills to make sure we have coverage on both of those projects in addition to our current upstream coverage checks. The way a user would interact with these would be something along the lines of: ```python config = SessionConfig().with_distributed(LocalhostWorkerResolver(worker_ports_from_env())) ``` One issue for `datafusion-distributed` specifically is I'm not 100% sure how to bring a third party resolver to the party. It's been a while since I dug into the code, so maybe it's already simple to do. I *think* Gabriel said it's not difficult now, so this is possibly a non-issue. ### Option 2 - Foreign plugin This would put almost all of the maintenance burden on the individual projects. The work in the `datafusion-python` repository is to finish up the upstream query planner work and then to expose these FFI objects. Also I had a branch where we allowed for the default codecs to make it so that well known executors could make it transparently through the FFI boundary, which would be needed by `datafusion-distributed`. Basically, we would check if executors are encodable with the default codec. If so, use it instead of making them foreign execs. The down side of this is that now each of these projects would need to maintain the python wrapper code and produce pypi wheels. It increases their burden for release since they are now releasing both rust crates and python wheels, and their developers need to become familiar with all of the PyO3 work, which sometimes requires *a lot* of changes from release to release. The way a user would interact with these would be something along the lines of: ```python ctx = SessionContext().with_query_planner( DataFusionDistributedPlanner(LocalhostWorkerResolver(worker_ports_from_env())) ) ``` There are two issues specific to `datafusion-distributed` that would need to be worked out. 1. Currently it uses downcasting of executors *a lot* throughtout the repository. I think we would need to do one of three things: (a) Change these to name based checks, which has the potential to be fickle / not robust (b) categorize each of these checks and add traits to the executor to return some kind of enum that tells if its a shuffle, repartition, etc (c) change the FFI crate to transparently convert these executors, like described above by using the default protobuf codec. 2. The current implementation relies on storing data in the config that are not string key/value pairs. I believe this is an anti-pattern anyways and technical debt, but it would have to be addressed right away. ## Request I would like to get other people's opinions besides Gabriel and myself. We're both very interested in making this work, but it's a big decision about how to go about supporting these. Tagging @milenkovicm since you're probably the person most involved on the Ballista 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]
