alamb commented on PR #22117: URL: https://github.com/apache/datafusion/pull/22117#issuecomment-4432397769
Hi @zhuqi-lucas -- I am sorry but I am really confused now. What I was trying to get at on https://github.com/apache/datafusion/pull/21976 is that I think we should reduce the risk of the rewrite by reusing the existing tests for EnforceSorting and EnforceDistribution I think the least risky way to avoid regressions is to reuse all the existing regression tests for those two passes (and make sure they pass with the new EnsureRequirements pass However upon additional review it seems like what actually happened was that you disconnected `EnsureSorting` and `EnsureDistribution` from the optimizer passes but left them in the code base so the old tests are still present. Then you / your agent recreated a new copy of many of the tests just in terms of `EnsureRequirements` (both in the new modules theselves like datafusion/physical-optimizer/src/ensure_requirements/mod.rs as well as in other modules like datafusion/physical-optimizer/src/ensure_requirements/new_tests.rs) Given all these new tests I was under the (incorrect) impression that you had also deleted the existing tests for EnsureSorting and EnsureDistribution So I guess maybe we already have the tests consolidated in `core/tests/physical_optimizer` and what is left to do is 1. remove the old EnsureDistribution and EnsureSorting passes 2. Update the existing tests to us EnsureRequirements 3. Remove any redundant tests that were added in https://github.com/apache/datafusion/pull/21976 Thank you for pushing this along -- I think this is one of the most complicated parts of the physical optimizer (so it is the hardest parts to change, but also one of the most important things to reduce compleixty) -- 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]
