jayzhan211 commented on code in PR #26657:
URL: https://github.com/apache/datafusion/pull/26657#discussion_r4430306901
##########
datafusion/sqllogictest/test_files/subquery.slt:
##########
@@ -4077,6 +4077,29 @@ DROP TABLE nested_agg_outer;
statement ok
DROP TABLE nested_agg_inner;
+# A set comparison in a projection without an alias must not change the
projection's
+# output field name. `RewriteSetComparison` replaces `x > ALL (…)` with a
`CASE` whose
+# display name differs from the field name, and rebuilding the projection from
the
+# expression names failed the optimizer's "schema is invariant" check with an
internal
+# error instead of returning the answer.
+query B
+select 2 > all (select v from (values (1), (0)) t(v));
+----
+true
+
+query BI
+select 2 > all (select v from (values (1), (0)) t(v)), 9 as k;
Review Comment:
Optional: could you add a case with a qualified column next to the set
comparison? It fails on `main` with the same internal error and passes here:
```sql
statement ok
create table t_qual(k int) as values (1), (2);
query IB
select t_qual.k, t_qual.k > all (select v from (values (1), (0)) s(v)) from
t_qual order by t_qual.k;
----
1 false
2 true
statement ok
drop table t_qual;
```
##########
datafusion/optimizer/src/decorrelate_predicate_subquery.rs:
##########
@@ -78,10 +78,21 @@ impl OptimizerRule for DecorrelatePredicateSubquery {
}
let original_projection = projection.clone();
+ // The projection's own schema is the authority for its output
field names.
+ // Earlier rules rewrite expressions with `map_expressions`, which
keeps the
+ // schema but can leave an expression whose display name no longer
matches its
+ // field name (`RewriteSetComparison` turns `x > ALL (…)` into a
`CASE` and does
Review Comment:
Since `RewriteSetComparison` is what leaves the stale name, could you fix it
there with `NamePreserver`, the same way `simplify_exprs` and `type_coercion`
wrap `map_expressions`? With this, the change to this file isn't needed: on
`main`'s version of this file plus the diff below, your new slt cases,
`subquery*.slt`, `merge_into.slt`, `null_aware_mark_join.slt` and the optimizer
unit tests all pass. It also stops this rule leaving mismatched names in
Aggregate/Window nodes.
```diff
+use datafusion_expr::expr_rewriter::NamePreserver;
...
+ let name_preserver = NamePreserver::new(&plan);
plan.map_expressions(|expr| {
+ let original_name = name_preserver.save(&expr);
expr.transform_up(|expr| rewrite_set_comparison(expr, &schema))
+ .map(|t| t.update_data(|expr| original_name.restore(expr)))
})
```
--
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]
