Skip to content

Commit 123747e

Browse files
SingletonYeSingletonYe
authored
fix: keep a projection's field names when decorrelating a predicate s… (#26657)
## Which issue does this PR close? No issue was filed; this body carries the report. ## Rationale for this change A set comparison in a projection that has no alias fails with an internal error: ```console > SELECT 2 > ALL (SELECT x FROM (VALUES (1),(0)) t(x)); Internal error: Assertion failed: compatible: Failed due to a difference in schemas: original schema: … Field { name: "Int64(2) > ALL", data_type: Boolean } … new schema: … Field { name: "CASE WHEN EXISTS THEN Boolean(false) …" } … ``` `DecorrelatePredicateSubquery` rebuilds a projection from its expressions, and it preserved the output field names from `Expr::schema_name()`. That is not the name the rebuilt projection uses, for two reasons: `schema_name()` keeps a column's qualifier (`t.k`) while the field is named after the bare column, and 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` does exactly that — it replaces `x > ALL (…)` with a `CASE WHEN EXISTS …` — so this rule rebuilt the projection under the stale name and the optimizer's schema-invariance check failed. `datafusion.optimizer.skip_failed_rules` defaults to false, so the user sees the internal error. Not on the release line: 55.2.0 does not contain #24972, so the same query still reports `Physical plan does not support logical expression Exists(…)` there. ## What changes are included in this PR? * `datafusion/optimizer/src/decorrelate_predicate_subquery.rs`: take the field names from the projection's own schema, and compare them against the name the rebuilt projection would use (`Expr::qualified_name`, i.e. the bare column name for a column, the alias name for an alias, the display name otherwise), so an alias is added only when it would really differ. Comparing against the schema name alone would alias a qualified column and lose its qualifier — the same check failing from the other side. * `datafusion/sqllogictest/test_files/subquery.slt`: three cases — unaliased, unaliased with a second projected column, and aliased. The aliased one is a control rather than a regression test: it passes on `main` too, and pins the alias arm of the name comparison. ## What is the testing strategy for this PR? The two unaliased `.slt` cases fail on `main` with `Optimizer rule 'decorrelate_predicate_subquery' failed` and pass with the change; the aliased case is the control described above. On the branch merged with `main`, `subquery.slt`, `subquery_projection.slt`, `null_aware_anti_join.slt`, `null_aware_mark_join.slt` and `prepare.slt` all pass with 0 errors, `cargo test -p datafusion-optimizer` is 951 + 29 + 5 passed and 0 failed, and `cargo fmt --all --check` is clean. ## Are there any user-facing changes? No.
1 parent 6110f74 commit 123747e

2 files changed

Lines changed: 51 additions & 1 deletion

File tree

‎datafusion/optimizer/src/rewrite_set_comparison.rs‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
//! `> ALL`) into boolean expressions built from `EXISTS` subqueries
77
//! that capture SQL three-valued logic.
88
9-
use crate::utils::merge_into_schema;
9+
use crate::utils::{NamePreserver, merge_into_schema};
1010
use crate::{OptimizerConfig, OptimizerRule};
1111
use datafusion_common::tree_node::{Transformed, TreeNode};
1212
use datafusion_common::{Column, DFSchema, ExprSchema, Result, ScalarValue, plan_err};
@@ -36,8 +36,19 @@ impl RewriteSetComparison {
3636
if let Some(merge_schema) = merge_into_schema(&plan)? {
3737
schema = merge_schema;
3838
}
39+
// This rule replaces `x > ALL (…)` with a `CASE WHEN EXISTS …`, whose display
40+
// name is not the name of the field it produces. Rewriting an expression
41+
// without preserving its name therefore changes the output schema, and the
42+
// optimizer's "the schema is invariant under optimization" check fails with an
43+
// internal error rather than returning the answer. `simplify_exprs` and
44+
// `type_coercion` preserve names around `map_expressions` for the same reason;
45+
// doing it here also stops this rule leaving mismatched names in `Aggregate`
46+
// and `Window` nodes, which a projection-only fix could not reach.
47+
let name_preserver = NamePreserver::new(&plan);
3948
plan.map_expressions(|expr| {
49+
let original_name = name_preserver.save(&expr);
4050
expr.transform_up(|expr| rewrite_set_comparison(expr, &schema))
51+
.map(|t| t.update_data(|expr| original_name.restore(expr)))
4152
})
4253
}
4354
}

‎datafusion/sqllogictest/test_files/subquery.slt‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4094,6 +4094,45 @@ DROP TABLE nested_agg_outer;
40944094
statement ok
40954095
DROP TABLE nested_agg_inner;
40964096

4097+
# A set comparison in a projection without an alias must not change the projection's
4098+
# output field name. `RewriteSetComparison` replaces `x > ALL (…)` with a `CASE` whose
4099+
# display name differs from the field name, and rebuilding the projection from the
4100+
# expression names failed the optimizer's "schema is invariant" check with an internal
4101+
# error instead of returning the answer.
4102+
query B
4103+
select 2 > all (select v from (values (1), (0)) t(v));
4104+
----
4105+
true
4106+
4107+
query BI
4108+
select 2 > all (select v from (values (1), (0)) t(v)), 9 as k;
4109+
----
4110+
true 9
4111+
4112+
# A control rather than a regression test: an aliased set comparison already keeps its
4113+
# name on `main`, so this one does not change with the fix. It pins the alias arm of the
4114+
# name comparison against a rule that starts renaming aliased projections.
4115+
query B
4116+
select 2 > all (select v from (values (1), (0)) t(v)) as aliased;
4117+
----
4118+
true
4119+
4120+
# A qualified column next to the set comparison is the other side of the same check:
4121+
# `Expr::schema_name` keeps the qualifier (`t_qual.k`) while the field is named after the
4122+
# bare column, so losing the qualifier breaks the schema just as adding a stale name does.
4123+
# Fails on `main` with the same internal error.
4124+
statement ok
4125+
create table t_qual(k int) as values (1), (2);
4126+
4127+
query IB
4128+
select t_qual.k, t_qual.k > all (select v from (values (1), (0)) s(v)) from t_qual order by t_qual.k;
4129+
----
4130+
1 false
4131+
2 true
4132+
4133+
statement ok
4134+
drop table t_qual;
4135+
40974136
# Regression tests for #25258 - Unaliased set comparison subqueries return wrong results
40984137

40994138
statement ok

0 commit comments

Comments
 (0)