alamb commented on PR #25688: URL: https://github.com/apache/datafusion/pull/25688#issuecomment-6014895358
> The TL;DR is that the existing optimizer rewrite implementation tries to recognize 3 possible aggregate plan shapes, and does so using a nested DFS algorithm. > However, there is a hidden constraint at the point where this rule runs, which means only one of those shapes is actually possible. With that constraint made explicit, the rewrite can be simplified to a straightforward pattern match without nested DFS. Thank you -- this is a very helpful example. I suspect the reason the rule look for three possible shapes is that they used to be created in earlier versions of DataFusion, so I wonder what type of boundary we could use to encode this in the future. > Now @zhuqi-lucas and @alamb seem more in favor of implementing the analyzer/optimizer split first, and treating a more general boundary mechanism as a follow-up. Yes, this accurately describes my preference, though I am happy to explore other alternatives and don't see any reason to rush this in. While the idea of a general boundary mechanism seems good in theory, I am somewhat skeptical of how useful it would be in practice. Specifically, I struggle to imagine what specific boundaries / invariants we could encode in an enumeration an enforce during planing other than the existing ones: 1. Schema matches 2. Declared required properties match actual input properties For example it is not clear to me the property described after `LimitedDistinctAggregation` in https://github.com/apache/datafusion/pull/26069/changes#r4194462241 is actually invariant won't ever change by all possible future optimizer passes, or even if we should try to enforce that as an invariant after subsequent passes I think my main concerns are: 1. Introducing a level of abstraction / API that isn't really used for anything other than the analyzer/optimizer split 2. Adding unnecessarily strict requirements on other optimizer passes > My current preference is to implement the more general optimizer boundary directly, to avoid introducing multiple mechanisms for the same underlying problem. > > I'd like to hear your thoughts before deciding on the next step. If you also think a general boundary mechanism is a plausible direction, I can put together a PoC this week. If we can come up with some other specific, useful boundaries / invariants this sounds like a good plan to me. -- 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]
