2010YOUY01 commented on PR #23599: URL: https://github.com/apache/datafusion/pull/23599#issuecomment-6072789069
> On your actual question. DataFusion is a library for building engines, so I don't think "a downstream pipeline produces this shape" is automatically out of scope. But the bar I'd want is that core could _plausibly_ produce the shape itself, and today it can't — the only thing keeping it from happening is an ordering nobody wrote down. So I'd rather make that explicit than quietly depend on it. I see, I think we are have different assumptions now - My assumption: the public API for optimizer is the ordered default rule list, extension should follow its existing assumptions - Yours: each individual optimizer rule itself is a public API, it should take care of any possible plan shape It would be great to find a middle ground, to enable more ergonomic extension, and avoid making this very strong assumption -- each rule should handle all possible shape is unrealistic I think. And the current practice (I think it's already quite common now) seem to be 'adding UTs on optimizer rules to guard downstream usecases', this should be a circular dependency between downstream and upstream. ### Issue 1: maintenance overhead The default rule list is a progressive refining process: - at rule 1, hash join might have 1 canonical plan shape - at rule 20, the semantically equivalent hash join have 5 variants If you have a rule to rewrite join, it's easier to pull it into the early phase. The assumption 'each rule should handle all possible shapes' can bloat the implementation complexity for early phase of optimizations. This PR do add extra logic into the implementation. ### Issue 2: hard to test The existing rules are tightly coupled, and the major test coverage is e2e test: run `sqllogictest` and apply all optimizer rules, then assert plan shape. To ensure this assumption is true, the test have to enumerate all combination/ordering of optimizer rules pipeline. So I think if we want to add such extra case handling, a practical check is ensure it's testable e2e. > * If it's useful to core, I have an e2e test that does discriminate (it fails on `main` and passes here) by inserting an extra `ProjectionPushdown` before `WindowTopN` and planning real SQL — though that still needs a non-default order, which is exactly your point. It should stronger then add a custom optimizer pipeline and only run a selected query. What I think is we can repeat all `slt` with different optimizer rule list. e.g. add a new optimizer configuration, might be the default list plus several rule for re-optimization, and such configuration might be more closely mapped to your downstream usage, and we can also check if such alternative otpimizer rule pipeline can actually bring extra capability, and get maintained more robustly. -- 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]
