fix: raise AmbiguousReference error for duplicate column names in subquery#21236
fix: raise AmbiguousReference error for duplicate column names in subquery#21236xiedeyantu wants to merge 5 commits intoapache:mainfrom
Conversation
…query derived tables
xudong963
left a comment
There was a problem hiding this comment.
Could you please add some slt tests? multiple joins (2, 3 etc) with duplicated columns
| let aliases = unique_field_aliases(plan.schema().fields()); | ||
| let is_projection_needed = aliases.iter().any(Option::is_some); | ||
|
|
||
| // Collect the set of unqualified field names that are ambiguous in this |
There was a problem hiding this comment.
Here marks a name as ambiguous when unique_field_aliases provides a rename for it. But unique_field_aliases renames all duplicates — so if there are 3 columns named age, the 2nd and 3rd get renamed.
The code collects field.name() for each renamed field, which means it collects "age" twice and puts it in the set. That works correctly due to HashSet dedup, but the first age (which was NOT renamed) is also ambiguous and only ends up in the set because one of the later duplicates shares its name. This is coincidentally correct but fragile — if unique_field_aliases ever changed to rename ALL duplicates (including the first), or if it renamed to something other than name:N, the logic could break. 🤔
A cleaner approach: count occurrences of each name and mark any name appearing 2+ times.
There was a problem hiding this comment.
@xudong963 Thank you very much for your review and excellent suggestions. My initial intention for this PR was to eliminate the unique naming convention—specifically, the name:N format—but it appears to play a critical role internally (it would be great if you could provide some further details on this). Consequently, I introduced an additional ambiguous_names field to track duplicate column names. To be honest, I felt this approach lacked elegance, but I couldn't come up with a better alternative at the time. Having reviewed your suggestions, however, I now believe they offer a superior solution; I will proceed to refactor this PR based on that approach. I will also add the corresponding SLT tests.
|
@xudong963 I have made the changes based on your suggestions and added tests. There is one failing item in the CI, but it appears unrelated to the current PR, as the |
Which issue does this PR close?
Rationale for this change
When two joined tables share a column with the same name (e.g.
age), aSELECT *inside a derived table subquery produces duplicate column names. Previously, referencing such a column by its unqualified name from the outer query silently succeeded instead of raising an ambiguity error, violating standard SQL semantics.What changes are included in this PR?
ambiguous_names: HashSet<String>field toDFSchemato track column names that are structurally ambiguous in a given schema context.DFSchema::with_ambiguous_names(builder) andDFSchema::ambiguous_names(accessor) methods.SubqueryAlias::try_new, afterunique_field_aliasesrenames duplicate columns to keep the Arrow schema valid, the original (pre-rename) names are collected intoambiguous_namesand attached to the output schema.DFSchema::qualified_field_with_unqualified_name, any lookup of an ambiguous name now immediately returnsSchemaError::AmbiguousReference.Column::normalize_with_schemas_and_ambiguity_check, even a single structural match is rejected when the containing schema has flagged the name as ambiguous.bad_extension_plannersnapshot test to include the newambiguous_namesfield in theDFSchemadebug output.Are these changes tested?
The existing
join_with_ambiguous_column,order_by_ambiguous_name, andgroup_by_ambiguous_nametests continue to pass. A new test case covering the reported scenario (select age from (SELECT * FROM a join b on a.aid = b.bid) as t) should be added todatafusion/sql/tests/sql_integration.rs.Are there any user-facing changes?
Yes. Queries that previously silently resolved an ambiguous column reference through a derived-table subquery will now receive a
Schema error: Ambiguous reference to unqualified field <name>error, consistent with standard SQL behavior and with how DataFusion already handles the same ambiguity at the direct JOIN level.