zhuqi-lucas commented on PR #25688: URL: https://github.com/apache/datafusion/pull/25688#issuecomment-5946484125
Thanks @alamb, and sorry for the long comments, I will keep them short from now on. > For example, if we had to initially treat JoinSelection as a PhysicalAnalyzerRule because it produces invalid plans we could do that initially (and keep the same effective ordering of passes as today, but split between Analyzer/Optimizer) > > Then in follow on PRs we could explore how to move JoinSelection into the OptimizerRules (e.g. by ensuring that it doesn't make plans invalid) That is what the current push does: `JoinSelection` runs as an analyzer rule, before enforcement, so nothing re-enforces any more; `EnsureRequirements` is one rule again; and the benchmark shows no regression. The one plan change left is the `window_topn.slt` case, which gains parallelism. It goes a bit beyond the smallest change in two places: `AggregateStatistics`, `LimitedDistinctAggregation` and `WindowTopN` already run after enforcement (with the small fixes that needed), and the debug-only `check_invariants` boundary check you pointed at is in. Happy to split either out if you prefer. The follow-ups I plan are exactly the ones you describe: moving `FilterPushdown` and then `JoinSelection` into the optimizer phase, once they can keep plans valid on their own. -- 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]
