milenkovicm commented on PR #18739: URL: https://github.com/apache/datafusion/pull/18739#issuecomment-3621900749
Hey @adriangb my comments are not focused on ballista at all. Ballista has, I believe, one rule which is forked from datafusion. But, be aware that there are other multi year codebases built on top of datafusion. I have been involved in a code base which has been built since DataFusion version 7, and have absorbed a lot of backward-incompatible changes over the years, and most of them had clean benefit(s) to the community. This one does not, at least it has not demonstrated, yet. My concern here is that the API has been broken without any valid reasons. Two new structures have been introduced to a public API; one of them, as you said in your comment, was clearly not necessary; the other does not demonstrate any valid reasons to be there as well, yet we're all expected to take them. Let's not take the path of least resistance and make changes to the public API if they are not really needed. So back to change, - As you have to provide a physical optimiser rule, can't you bring all other necessary "things" as rule properties? So your optimiser has internally whatever you need in `OptimiserContext`. - Even `SessionConfig` as an optimiser parameter is hard to sell, as an optimiser rule has the same lifecycle as session state/session config, and optimiser properties may be the location to keep extra data. - If you need to keep a reference to an optimiser, can it be kept as a session config extension (optimiser rule may not be seen as a config, maybe we need a session state extension instead to keep a reference to)? I hope this helps , -- 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]
