2010YOUY01 commented on PR #23599: URL: https://github.com/apache/datafusion/pull/23599#issuecomment-6058029580
I tried to add this new `slt` to main, and they're passing 🤔 If it's regression test, it's expected to fail (not optimized to windowTopK) without this PR. This claim should be right https://github.com/apache/datafusion/pull/23599#issuecomment-6011901158, there is a implicit 'no-embedded-projection' zone in the default optimizer rule list. And `WindowTopN` is assuming no projection in filter currently, to simplify its implementation. ```rust let rules: Vec<Arc<dyn PhysicalOptimizerRule + Send + Sync>> = vec![ // ---- BEGIN: no-embedded-projection zone ---- Arc::new(OutputRequirements::new_add_mode()), // ...... Arc::new(WindowTopN::new()), // ...... // ---- END: no-embedded-projection zone ---- Arc::new(ProjectionPushdown::new()), ] ``` This hidden convention itself is not a blocker of this PR, but the real issue is, fix and improvements should verifiable with e2e tests, and this optimizer improvement can only be verified with UT taking a plan shape, that can't get produced from the default optimizer rule order. It feel a bit like DataFusion core being extended to adapt downstream custom optimizer pipeline, I'm uncertain if it's expected. Would love to hear your thoughts on this. -- 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]
