alamb commented on PR #25688: URL: https://github.com/apache/datafusion/pull/25688#issuecomment-5936142091
> The optimizer-phase delta is join_selection +172.2, EnsureRequirements -133.4, OptimizeSorts +29.8. So the extra time is inside JoinSelection, and it is the enforce_distribution_requirements call it makes after rewriting. That function runs twice per plan here and is 22% of planning. Splitting the two rules is not where it went: on merge-base EnsureRequirements already does distribution and sorting as two separate bottom-up walks, so merging them removes no traversal, and OptimizeSorts at 29.8 us cannot account for a 210 us regression even at zero. So one way to bring back performance then is to update JoinSelection so it doesn't have to call `enforce_distribution_requirements` (it can fix the distribution internally if it changes the plan tree). Sorry I find it really hard to read large / wall of comments, so you may have already said this farther down > To be explicit, that makes JoinSelection an analyzer rule rather than an optimizer one. It is what you floated earlier in this review, "put JoinSelection as an analyzer rule (as strange as that is)", and I think it is less strange than it sounds: PartitionMode::Auto is not executable at all, HashJoinExec::execute returns a plan error on it, so resolving the mode is a correctness step under the definition you gave. I guess what I am advocating is trying to introduce PhysicalAnalyzerRule with the smallest number of other changes as possible. 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) Those steps I think are more self contained and easier to review -- 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]
