Attach diagnostic information for duplicate table name error#20720
Attach diagnostic information for duplicate table name error#20720buraksenn wants to merge 7 commits intoapache:mainfrom
Conversation
| } | ||
| }; | ||
|
|
||
| let mut alias_spans: HashMap<String, Option<Span>> = HashMap::new(); |
There was a problem hiding this comment.
I think this currently extracts only the final identifier for unaliased table factors. That means something like catalog1.schema.person and catalog2.schema.person would both be tracked as person and get flagged as duplicates.
However, DataFusion's scan qualifier uses the full normalized TableReference, so these should actually be treated as distinct.
Would it make sense to reuse object_name_to_table_reference(name.clone())?.to_string() here, or otherwise preserve the full relation identity for unaliased tables?
| @@ -690,21 +694,75 @@ impl<S: ContextProvider> SqlToRel<'_, S> { | |||
| self.plan_table_with_joins(input, planner_context) | |||
There was a problem hiding this comment.
It looks like the duplicate alias diagnostic is only applied in the multi-entry comma FROM path. A single TableWithJoins still goes straight to plan_table_with_joins, so explicit joins skip this check.
For example, (SELECT 1 AS a) AS t JOIN (SELECT 2 AS b) AS t ON true would not be caught here. Depending on the columns, this could either fall back to a schema error or even plan successfully with duplicate t aliases.
Could we move the alias and span tracking into the relation or join planning path so both comma joins and explicit JOINs go through the same validation and produce consistent diagnostics?
| |t: &TableWithJoins| -> Option<(String, Option<Span>)> { | ||
| match &t.relation { | ||
| TableFactor::Table { alias: Some(a), .. } | ||
| | TableFactor::Derived { alias: Some(a), .. } |
There was a problem hiding this comment.
This extract_table_name closure is doing a few different things at once, like relation name normalization, alias span lookup, and relation kind filtering.
Once the JOIN coverage is addressed, would it be worth pulling this into a small helper near relation planning? It might make the naming rules easier to follow and extend as more table factor variants are added.
|
Thanks for the detailed review @kosiew. I'll start working on these today |
3 participants
Which issue does this PR close?
Diagnosticto "duplicate table name" error #14436Rationale for this change
This is part of #14429 epic to add diagnostic to more errors.
What changes are included in this PR?
This PR adds optional span with the CTE and uses those spans to best effort attach diagnostics
Are these changes tested?
Are there any user-facing changes?
Yes users can call diagnostic methods to get detailed information about errors