kosiew commented on code in PR #24686: URL: https://github.com/apache/datafusion/pull/24686#discussion_r3902481846
########## datafusion/optimizer/src/optimize_projections/mod.rs: ########## Review Comment: I think this equality fast path is the underlying correctness issue. Two consecutive `i + 1 AS i` projections can have equal `Expr`s, but they are not idempotent. Dropping one of them also drops one evaluation and changes the result. This fast path predates the iterative merge, so removing the merge loop only avoids triggering the problem for some shallow chains rather than fixing the root cause. Could we keep the iterative merge and make this fast path explicitly check that the projection is safe to elide? For example, we could limit it to column projections and aliases of columns. I would also add a focused metadata-bearing alias test here to make sure the guard preserves alias metadata. ########## datafusion/optimizer/src/optimize_projections/mod.rs: ########## Review Comment: I don't think we should remove the iterative merge here. Without it, projection chains need additional outer optimizer passes to collapse. With `max_passes = 1`, deeper chains retain consecutive projections, and sufficiently deep chains can still retain them with the default `max_passes = 3`. The extra pass-through projection accepted by the updated optimizer integration snapshot appears to be a consequence of removing this loop rather than a plan change we actually want. Could we restore the iterative merge after fixing the equality fast path below? That should preserve the one-invocation projection-collapse behavior without reintroducing the incorrect elision. ########## datafusion/sqllogictest/test_files/projection.slt: ########## @@ -217,6 +217,74 @@ SELECT column1 as a from (values (1), (2)) f where f.column1 = 2; ---- 2 +# Regression: one optimizer pass must preserve anonymous nested projections. +statement ok +CREATE TABLE nested_projection(i INT); + +statement ok +INSERT INTO nested_projection VALUES (3), (4), (5); + +statement ok +SET datafusion.optimizer.max_passes = 1; + +query I rowsort Review Comment: Thanks for adding regression coverage here. I think we need to make it a little stronger, because depth 3 with `max_passes = 1` covers the case this patch happens to repair but does not catch the remaining equality-elision failure in deeper chains. Could we extend this to depth 5 or 6 and test both `max_passes = 1` and the default optimizer configuration? I would also add a focused optimizer unit test for structurally equal but non-idempotent expressions, plus a metadata-bearing alias case. It would be useful to assert that the optimized projection chain collapses as well, so the test protects both correctness and the plan-shape behavior provided by the iterative merge. -- 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]
