kosiew commented on code in PR #24686:
URL: https://github.com/apache/datafusion/pull/24686#discussion_r3879915314
##########
datafusion/optimizer/src/optimize_projections/mod.rs:
##########
@@ -522,30 +522,6 @@ fn optimize_subqueries(
/// - `Ok(None)`: Signals that merge is not beneficial (and has not taken
place).
/// - `Err(error)`: An error occurred during the function call.
fn merge_consecutive_projections(proj: Projection) ->
Result<Transformed<Projection>> {
Review Comment:
There is also a planning regression from removing this loop. On a 12-deep
projection chain with `max_passes = 1`, `main` collapses the optimized logical
plan to 1 `Projection:` node, while this PR leaves 7.
This also matters with the default `max_passes = 3`, where sufficiently deep
chains can still leave consecutive projections in the final plan. The
integration snapshot changed by this PR is one example of that extra layer
becoming part of the expected plan.
If the equality fast path is guarded instead, as suggested in the other
comment, the loop can stay and this regression goes away as well.
##########
datafusion/optimizer/src/optimize_projections/mod.rs:
##########
@@ -522,30 +522,6 @@ fn optimize_subqueries(
/// - `Ok(None)`: Signals that merge is not beneficial (and has not taken
place).
/// - `Err(error)`: An error occurred during the function call.
fn merge_consecutive_projections(proj: Projection) ->
Result<Transformed<Projection>> {
Review Comment:
I don't think the iterative merge is the root cause of the wrong results
here. Removing it fixes the depth-3 case, but the same query still returns
incorrect results from depth 4 onward with the default `max_passes = 3`.
The issue appears to be the fast path below this code:
```rust
if prev_projection.expr == expr {
...
}
```
That check drops the current projection whenever the expressions are
structurally equal. This is only safe when the projection is idempotent. For
example, `i + 1 AS i` over a child that also exposes `i` compares equal
structurally, but evaluating it again changes the value, so dropping the
projection removes one `+ 1` from the query.
I confirmed this by keeping the iterative merge and guarding the fast path
so it only applies to trivial projections:
```rust
let all_trivial = expr.iter().all(|e| match e {
Expr::Column(_) => true,
Expr::Alias(a) => matches!(a.expr.as_ref(), Expr::Column(_)),
_ => false,
});
if all_trivial && prev_projection.expr == expr {
```
With that change, while retaining the loop, depths 1 through 12 return the
expected results with `max_passes` set to 1, 3, or 10. I also get 763/763
passing tests from `cargo test -p datafusion-optimizer --lib`, 504/504 files
from the full sqllogictest suite, and this PR's regression test passes.
Could we fix the fast-path condition instead of removing the iterative merge?
##########
datafusion/sqllogictest/test_files/projection.slt:
##########
@@ -217,6 +217,39 @@ 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:
Could we extend this regression beyond depth 3? Depth 3 is the particular
case this change fixes, but depth 4 still returns a wrong result with the
default optimizer pass count.
I suggest testing at least depth 5 or 6 and checking the results with both
`max_passes = 1` and the default setting. That would make the test protect the
general nested-projection behavior rather than one depth that happens to pass
with this implementation.
It may also be useful to check the optimized plan with `EXPLAIN`, so the
test covers both correct values and the projection-collapse behavior.
##########
datafusion/optimizer/tests/optimizer_integration.rs:
##########
@@ -792,9 +792,10 @@ fn extension_node_does_not_block_projection_pruning() ->
Result<()> {
OpaqueRequirementsExtension
Sort: t.a ASC NULLS FIRST, t.ts ASC NULLS FIRST
Projection: t.a, CAST(t.ts AS Timestamp(ms, "UTC")) AS ts
- Filter: __common_expr_3 > TimestampMillisecond(1000, Some("UTC"))
AND __common_expr_3 < TimestampMillisecond(2000, Some("UTC"))
- Projection: CAST(t.ts AS Timestamp(ms, "UTC")) AS __common_expr_3,
t.a, t.ts
- TableScan: t projection=[a, ts], partial_filters=[CAST(t.ts AS
Timestamp(ms, "UTC")) > TimestampMillisecond(1000, Some("UTC")), CAST(t.ts AS
Timestamp(ms, "UTC")) < TimestampMillisecond(2000, Some("UTC"))]
+ Projection: t.a, t.ts
Review Comment:
I don't think we should accept this extra pass-through `Projection: t.a,
t.ts` as the new expected plan. It looks like a consequence of removing the
iterative merge rather than an independently necessary plan change.
When I keep the loop and guard the equality fast path, the previous tighter
plan remains valid. If we take that approach, I think this snapshot change
should be reverted.
##########
datafusion/sqllogictest/test_files/projection.slt:
##########
@@ -217,6 +217,39 @@ 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:
One additional test suggestion: I think a focused unit test in
`optimize_projections` would be more valuable than adding this pattern to the
long chained-projection benchmark. A benchmark would measure planning time, but
it would not directly protect against this correctness bug.
A unit test could specifically cover a non-idempotent projection that
compares structurally equal to its input. It would also be useful to include a
metadata-bearing alias variant, since alias metadata interacts with the
trimming logic in `merge_consecutive_projections`.
--
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]
