adriangb commented on code in PR #25412:
URL: https://github.com/apache/datafusion/pull/25412#discussion_r4088747354
##########
datafusion/optimizer/src/extract_leaf_expressions.rs:
##########
@@ -637,6 +637,24 @@ impl<'a> LeafExpressionExtractor<'a> {
}
}
+/// The way `schema` names `col`, or `None` when it does not hold it
unambiguously.
+///
+/// A qualified column is taken as it stands. An unqualified one is matched on
name alone
+/// and comes back carrying the qualifier the schema gives that field, so a
column pushed
+/// into a projection reads as the input spells it. A name the schema holds
more than once
+/// resolves to nothing rather than to an arbitrary one of them.
+fn resolve_against(schema: &DFSchema, col: &Column) -> Option<Column> {
+ match &col.relation {
+ Some(relation) => schema
+ .has_column_with_qualified_name(relation, &col.name)
+ .then(|| col.clone()),
+ None => schema
+ .qualified_field_with_unqualified_name(&col.name)
+ .ok()
+ .map(|(qualifier, field)| Column::new(qualifier.cloned(),
field.name())),
+ }
+}
Review Comment:
**Suggestion 1: one lookup for both arms.**
`DFSchema::qualified_field_from_column` already does the qualified and the
unqualified lookup, and `Column: From<(Option<&TableReference>, &FieldRef)>`
exists.
| | Current | Suggested |
|---|---|---|
| Qualified arm | `has_column_with_qualified_name` (exact
`TableReference::eq`), returns the caller's spelling |
`index_of_column_by_name` (`resolved_eq`), returns the schema's spelling |
| Unqualified arm | `qualified_field_with_unqualified_name` | same |
| Result | two spellings can still differ (for example `public.t.c` vs
`t.c`) | always the input's spelling, so `existing_cols.contains` compares like
with like |
I ran the optimizer tests with this change: 57 passed.
The doc also claims a duplicate name resolves to `None`. When exactly one of
the matches is unqualified, the lookup returns that one (see
`qualified_field_with_unqualified_name`). The suggested doc says what the code
does.
```suggestion
/// The way `schema` names `col`, or `None` when `schema` does not hold it
/// or the name is ambiguous.
///
/// The result always carries the qualifier the schema gives the field, so
/// two spellings of the same input column compare equal, and a column pushed
/// into a projection reads as the input spells it.
fn resolve_against(schema: &DFSchema, col: &Column) -> Option<Column> {
schema.qualified_field_from_column(col).ok().map(Column::from)
}
```
##########
datafusion/optimizer/src/extract_leaf_expressions.rs:
##########
@@ -702,27 +720,31 @@ fn build_extraction_projection_impl(
// than target_schema (the projection's output) because columns
produced
// by alias expressions (e.g., CSE's __common_expr_N) exist in the
output but
// not the input, and cannot be added as pass-through Column
references.
+ //
+ // Both sides of that check are read in the input's own spelling. A
column can
+ // arrive here unqualified while the input names it `t.c`, and the
other way round:
+ // a projection of bare names over a qualified input is what
eliminating one side
+ // of a union leaves behind. Resolving both sides lets a pass-through
the
+ // projection already carries match the one about to be added, and
pushes the one
+ // that is genuinely new under the name the input gives it. Left
unresolved, the
+ // merged projection would hold `t.c` and a bare `c` together, which
+ // `Projection::try_new` rejects as ambiguous. A same-name alias such
as
+ // `t.c AS c` is a pass-through as well, and counts the same as a bare
`t.c`.
Review Comment:
**Suggestion 2 (nit): shorter comment.** The existing paragraph above is 5
lines; this one is 9. The same content fits in fewer lines:
```suggestion
// Compare both sides in the input's spelling (see
`resolve_against`).
// Without this, a bare `c` and a qualified `t.c` do not match, and
the
// merged projection holds both, which `Projection::try_new` rejects
as
// ambiguous. Eliminating the empty side of a union makes this shape.
// A same-name alias (`t.c AS c`) counts as a pass-through of `t.c`.
```
##########
datafusion/sqllogictest/test_files/struct.slt:
##########
@@ -1803,3 +1803,28 @@ drop view struct_ctor_view;
statement ok
drop table struct_ctor_null;
+
+# Merging an extraction projection into a projection whose output lost its
+# qualifier. Eliminating the empty side of the union leaves a projection of
+# bare column names over a qualified input, and the merge used to add the
+# pass-through columns under those bare names beside the qualified ones the
+# projection already carried, which is an ambiguous schema.
+statement ok
+create table leaf_merge_source(v int, s struct<a int>, env varchar) as values
(1, {a: 10}, 'prod'), (2, {a: 20}, 'dev');
Review Comment:
**Suggestion 3: lock the plan shape.** The result check passes if a future
change to union elimination stops making this shape. Then the test no longer
covers the merge. An `EXPLAIN` makes that visible. I ran this block with
`--complete` on the PR head, and then without `--complete`: it passes.
```suggestion
create table leaf_merge_source(v int, s struct<a int>, env varchar) as
values (1, {a: 10}, 'prod'), (2, {a: 20}, 'dev');
statement ok
set datafusion.explain.logical_plan_only = true;
query TT
explain with samples as (
select v, s, env from leaf_merge_source
),
expanded as (
select v, s, env from samples
union all
select v, s, env from samples where 1 = 2
)
select env, sum(s['a']) from expanded group by env order by env;
----
logical_plan
01)Sort: expanded.env ASC NULLS LAST
02)--Projection: expanded.env, sum(__datafusion_extracted_1) AS
sum(expanded.s[a])
03)----Aggregate: groupBy=[[expanded.env]],
aggr=[[sum(CAST(__datafusion_extracted_1 AS Int64))]]
04)------SubqueryAlias: expanded
05)--------SubqueryAlias: samples
06)----------Projection: leaf_merge_source.env, get_field(s, Utf8("a")) AS
__datafusion_extracted_1
07)------------TableScan: leaf_merge_source projection=[s, env]
statement ok
set datafusion.explain.logical_plan_only = false;
```
Optional: `projection_pushdown.slt` already holds the other leaf-pushdown
plan tests, so this block can also go there.
--
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]
