alamb commented on issue #26171: URL: https://github.com/apache/datafusion/issues/26171#issuecomment-6111476758
> If we restrict the contract to guarantee validity, but not optimization coverage: > * The custom pipeline is guaranteed to be runnable. > * However, it might miss some optimization opportunities. For example, in the default pipeline, `rule1` might only encounter certain canonical plan shapes, while the custom pipeline could introduce more diverse plan variations at `rule1`. I agree with this description > Custom pipelines should preserve correctness, but are not guaranteed equivalent optimization coverage. Yes > Additional optimization coverage should be justified and tested as an upstream feature. Yes > Later, a downstream project introduces a custom pipeline that produces many equivalent but non-canonical join shapes. It then requests that the built-in rule be extended to recognize 10 additional shapes. I feel like this is the core challenge that we need to address -- how to decide to push back (or accept) code to handle shapes that the built in pipeline never covers. > > Here are two in-the-wild examples of downstream extension requirements. In the context of this issue, addressing (1) seems uncontroversial, while (2) may require further justification. > > 1. Downstream requirement to avoid internal errors > > * [JoinSelection should skip HashJoinExec nodes that already carry a dynamic filter instead of failing in swap_inputs #26106](https://github.com/apache/datafusion/issues/26106) In my mind, this is not really a new requirement on the optimizer, it is a requirement from HashJoinExec itself (which was being triggered by the optimizer). I do think the requirements/semantics of individual ExecutionPlan nodes could be much better > 2. Downstream requirement for broader optimization coverage > * [fix(physical-optimizer): decline the WindowTopN rewrite when the FilterExec carries a fetch #23599](https://github.com/apache/datafusion/pull/23599) The current PR as proposed I don't think is a request broader optimization coverage -- it looks to me like a bugfix, but it also looks like it has changed significantly since when @zhuqi-lucas first proposed it I do see the code / request for extending WindowExec pushdown when FilterExec has an embedded projection (which the default optimizer rule order never exposes) -- 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]
