Skip to content

Commit 12a42d9

Browse files
SingletonYeSingletonYe
committed
review: reuse Expr::qualified_name for the projection field name
`Expr::qualified_name` implements and documents exactly the three arms the local helper had, so the helper was a second copy of the field-naming rule that a later change to either one could drift from. Zip the projection's expressions with its schema's field names so the pairing is structural instead of positional, and say in subquery.slt that the aliased case is a control rather than a regression test. Verified 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 exit 0, and cargo test -p datafusion-optimizer is 951 + 29 + 5 passed, 0 failed.
1 parent aea1cf9 commit 12a42d9

2 files changed

Lines changed: 10 additions & 19 deletions

File tree

‎datafusion/optimizer/src/decorrelate_predicate_subquery.rs‎

Lines changed: 7 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -60,19 +60,6 @@ impl DecorrelatePredicateSubquery {
6060
}
6161
}
6262

63-
/// The name a projection rebuilt from `expr` gives that expression's field.
64-
///
65-
/// `Expr::schema_name` is not that name for a column reference: it keeps the
66-
/// qualifier (`t.k`), while the field is named after the bare column (`k`) and
67-
/// stores the qualifier separately.
68-
fn projected_field_name(expr: &Expr) -> String {
69-
match expr {
70-
Expr::Alias(alias) => alias.name.clone(),
71-
Expr::Column(column) => column.name.clone(),
72-
other => other.schema_name().to_string(),
73-
}
74-
}
75-
7663
impl OptimizerRule for DecorrelatePredicateSubquery {
7764
fn rewrite(
7865
&self,
@@ -102,12 +89,10 @@ impl OptimizerRule for DecorrelatePredicateSubquery {
10289
.schema
10390
.fields()
10491
.iter()
105-
.map(|field| field.name().to_string())
106-
.collect::<Vec<_>>();
92+
.map(|field| field.name().to_string());
10793
let mut cur_input = Arc::unwrap_or_clone(projection.input);
10894
let mut rewritten_exprs = Vec::with_capacity(projection.expr.len());
109-
for (index, expr) in projection.expr.into_iter().enumerate() {
110-
let original_name = &field_names[index];
95+
for (expr, original_name) in projection.expr.into_iter().zip(field_names) {
11196
let (new_input, mut rewritten_expr) =
11297
rewrite_inner_subqueries(cur_input, expr, config, true, true)?;
11398
if has_subquery(&rewritten_expr) {
@@ -117,8 +102,11 @@ impl OptimizerRule for DecorrelatePredicateSubquery {
117102
}
118103
cur_input = new_input;
119104

120-
if projected_field_name(&rewritten_expr) != *original_name {
121-
rewritten_expr = rewritten_expr.alias(original_name.as_str());
105+
// `Expr::qualified_name` is the name the rebuilt projection gives this
106+
// expression: the bare column name for a column reference, the alias name
107+
// for an alias, and the display name otherwise.
108+
if rewritten_expr.qualified_name().1 != original_name {
109+
rewritten_expr = rewritten_expr.alias(original_name);
122110
}
123111
rewritten_exprs.push(rewritten_expr);
124112
}

‎datafusion/sqllogictest/test_files/subquery.slt‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4092,6 +4092,9 @@ select 2 > all (select v from (values (1), (0)) t(v)), 9 as k;
40924092
----
40934093
true 9
40944094

4095+
# A control rather than a regression test: an aliased set comparison already keeps its
4096+
# name on `main`, so this one does not change with the fix. It pins the alias arm of the
4097+
# name comparison against a rule that starts renaming aliased projections.
40954098
query B
40964099
select 2 > all (select v from (values (1), (0)) t(v)) as aliased;
40974100
----

0 commit comments

Comments
 (0)