discord9 commented on PR #24686: URL: https://github.com/apache/datafusion/pull/24686#issuecomment-5491724700
> @discord9, > > Thanks for working on this. I took another look at the latest version and re-verified the behavior against the current head. I think there are still a few issues we need to address before merging. > > The main concern is that removing the iterative merge avoids the reported failure for the shallow case, but it does not address the underlying structural-equality fast path. That path can still remove a projection that needs to be evaluated. Removing the loop also introduces a projection-collapse regression for deeper chains. > > I left the specific changes I think are needed inline. In short, I suggest keeping the iterative merge, guarding the equality fast path so it only elides projections that are safe to elide, and extending the regression coverage to deeper chains and the default optimizer pass count. > > Thanks again for digging into this. Thanks for the review, I think [fa440be](https://github.com/apache/datafusion/pull/24686/commits/fa440be3a024f6a918fc51640df3a289ddc707b8) should address most of the review comment? -- 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]
