You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "buraksenn (via GitHub)" <gi...@apache.org> on 2026/03/05 12:52:14 UTC
[PR] Attach diagnostic information for duplicate table name error [datafusion]
buraksenn opened a new pull request, #20720:
URL: https://github.com/apache/datafusion/pull/20720
## Which issue does this PR close?
- Closes #14436
## Rationale 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?
- I've run existing SQL tests to make sure not break any functionality
- Added new tests for diagnostics
## Are there any user-facing changes?
Yes users can call diagnostic methods to get detailed information about errors
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Attach diagnostic information for duplicate table name error [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #20720:
URL: https://github.com/apache/datafusion/pull/20720#discussion_r3093787496
##########
datafusion/sql/src/select.rs:
##########
@@ -690,21 +694,75 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
self.plan_table_with_joins(input, planner_context)
Review Comment:
I've missed other paths as you've described. I think latest changes cover this. Thanks for catching it @kosiew
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Attach diagnostic information for duplicate table name error [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #20720:
URL: https://github.com/apache/datafusion/pull/20720#discussion_r3079091677
##########
datafusion/sql/src/select.rs:
##########
@@ -690,21 +694,75 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
self.plan_table_with_joins(input, planner_context)
}
_ => {
+ let extract_table_name =
+ |t: &TableWithJoins| -> Option<(String, Option<Span>)> {
+ match &t.relation {
+ TableFactor::Table { alias: Some(a), .. }
+ | TableFactor::Derived { alias: Some(a), .. }
Review 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.
##########
datafusion/sql/src/select.rs:
##########
@@ -690,21 +694,75 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
self.plan_table_with_joins(input, planner_context)
Review 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?
##########
datafusion/sql/src/select.rs:
##########
@@ -690,21 +694,75 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
self.plan_table_with_joins(input, planner_context)
}
_ => {
+ let extract_table_name =
+ |t: &TableWithJoins| -> Option<(String, Option<Span>)> {
+ match &t.relation {
+ TableFactor::Table { alias: Some(a), .. }
+ | TableFactor::Derived { alias: Some(a), .. }
+ | TableFactor::Function { alias: Some(a), .. }
+ | TableFactor::UNNEST { alias: Some(a), .. }
+ | TableFactor::NestedJoin { alias: Some(a), .. } => {
+ let span = Span::try_from_sqlparser_span(a.name.span);
+ let name =
+ self.ident_normalizer.normalize(a.name.clone());
+ Some((name, span))
+ }
+ TableFactor::Table {
+ name, alias: None, ..
+ } => {
+ let span =
+ Span::try_from_sqlparser_span(t.relation.span());
+ let table_name = name
+ .0
+ .iter()
+ .filter_map(|p| p.as_ident())
+ .map(|id| self.ident_normalizer.normalize(id.clone()))
+ .next_back()?;
+ Some((table_name, span))
+ }
+ _ => None,
+ }
+ };
+
+ let mut alias_spans: HashMap<String, Option<Span>> = HashMap::new();
Review 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?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Attach diagnostic information for duplicate table name error [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #20720:
URL: https://github.com/apache/datafusion/pull/20720#discussion_r3093783598
##########
datafusion/sql/src/select.rs:
##########
@@ -690,21 +694,75 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
self.plan_table_with_joins(input, planner_context)
}
_ => {
+ let extract_table_name =
+ |t: &TableWithJoins| -> Option<(String, Option<Span>)> {
+ match &t.relation {
+ TableFactor::Table { alias: Some(a), .. }
+ | TableFactor::Derived { alias: Some(a), .. }
+ | TableFactor::Function { alias: Some(a), .. }
+ | TableFactor::UNNEST { alias: Some(a), .. }
+ | TableFactor::NestedJoin { alias: Some(a), .. } => {
+ let span = Span::try_from_sqlparser_span(a.name.span);
+ let name =
+ self.ident_normalizer.normalize(a.name.clone());
+ Some((name, span))
+ }
+ TableFactor::Table {
+ name, alias: None, ..
+ } => {
+ let span =
+ Span::try_from_sqlparser_span(t.relation.span());
+ let table_name = name
+ .0
+ .iter()
+ .filter_map(|p| p.as_ident())
+ .map(|id| self.ident_normalizer.normalize(id.clone()))
+ .next_back()?;
+ Some((table_name, span))
+ }
+ _ => None,
+ }
+ };
+
+ let mut alias_spans: HashMap<String, Option<Span>> = HashMap::new();
Review Comment:
I've missed this actually and added test case and properly use TableReferences
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Attach diagnostic information for duplicate table name error [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #20720:
URL: https://github.com/apache/datafusion/pull/20720#discussion_r2895368717
##########
datafusion/sql/src/select.rs:
##########
@@ -690,21 +694,70 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
self.plan_table_with_joins(input, planner_context)
}
_ => {
+ let extract_table_name =
+ |t: &TableWithJoins| -> Option<(String, Option<Span>)> {
+ let span = Span::try_from_sqlparser_span(t.relation.span());
+ match &t.relation {
+ TableFactor::Table { alias: Some(a), .. } => {
+ let name =
+ self.ident_normalizer.normalize(a.name.clone());
+ Some((name, span))
+ }
+ TableFactor::Table {
+ name, alias: None, ..
+ } => {
+ let table_name = name
+ .0
+ .iter()
+ .filter_map(|p| p.as_ident())
+ .map(|id| self.ident_normalizer.normalize(id.clone()))
+ .next_back()?;
+ Some((table_name, span))
+ }
+ _ => None,
+ }
+ };
+
+ let mut alias_spans: HashMap<String, Option<Span>> = HashMap::new();
+
let mut from = from.into_iter();
+ let first = from.next().unwrap();
+
+ if let Some((name, span)) = extract_table_name(&first) {
+ alias_spans.entry(name).or_insert(span);
+ }
+
+ let mut left = LogicalPlanBuilder::from(
+ self.plan_table_with_joins(first, planner_context)?,
+ );
- let mut left = LogicalPlanBuilder::from({
- let input = from.next().unwrap();
- self.plan_table_with_joins(input, planner_context)?
- });
let old_outer_from_schema = {
let left_schema = Some(Arc::clone(left.schema()));
planner_context.set_outer_from_schema(left_schema)
};
for input in from {
- // Join `input` with the current result (`left`).
+ let current_name = extract_table_name(&input);
+
+ if let Some((ref name, ref span)) = current_name {
+ alias_spans.entry(name.clone()).or_insert(*span);
Review Comment:
I think this should be done after the closure for map_err at line 747 because it clashes with `prior_span`.
I.e. first run the body of the closure, then insert this entry and finally execute the cross join.
Something like:
```rust
if let Some((ref name, current_span)) = current_name {
if let Some(first_span) = alias_spans.get(name).copied() {
let diagnostic = Diagnostic::new_error(
"duplicate table alias in FROM clause",
current_span,
)
.with_note("first defined here", first_span);
return plan_err!("duplicate table alias in FROM clause")
.map_err(|e| e.with_diagnostic(diagnostic));
}
alias_spans.insert(name.clone(), current_span);
}
left = left.cross_join(right)?;
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Attach diagnostic information for duplicate table name error [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on PR #20720:
URL: https://github.com/apache/datafusion/pull/20720#issuecomment-4244669762
Thanks for the detailed review @kosiew. I'll start working on these today
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Attach diagnostic information for duplicate table name error [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #20720:
URL: https://github.com/apache/datafusion/pull/20720#issuecomment-4714234952
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Attach diagnostic information for duplicate table name error [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #20720:
URL: https://github.com/apache/datafusion/pull/20720#discussion_r3093781931
##########
datafusion/sql/src/select.rs:
##########
@@ -690,21 +694,75 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
self.plan_table_with_joins(input, planner_context)
}
_ => {
+ let extract_table_name =
+ |t: &TableWithJoins| -> Option<(String, Option<Span>)> {
+ match &t.relation {
+ TableFactor::Table { alias: Some(a), .. }
+ | TableFactor::Derived { alias: Some(a), .. }
Review Comment:
It makes sense since especially now it is used in multiple places. I've extracted this thanks
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Attach diagnostic information for duplicate table name error [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #20720:
URL: https://github.com/apache/datafusion/pull/20720#discussion_r2895761682
##########
datafusion/sql/src/select.rs:
##########
@@ -690,21 +694,70 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
self.plan_table_with_joins(input, planner_context)
}
_ => {
+ let extract_table_name =
+ |t: &TableWithJoins| -> Option<(String, Option<Span>)> {
+ let span = Span::try_from_sqlparser_span(t.relation.span());
+ match &t.relation {
+ TableFactor::Table { alias: Some(a), .. } => {
+ let name =
+ self.ident_normalizer.normalize(a.name.clone());
+ Some((name, span))
+ }
+ TableFactor::Table {
+ name, alias: None, ..
+ } => {
+ let table_name = name
+ .0
+ .iter()
+ .filter_map(|p| p.as_ident())
+ .map(|id| self.ident_normalizer.normalize(id.clone()))
+ .next_back()?;
+ Some((table_name, span))
+ }
+ _ => None,
+ }
+ };
+
+ let mut alias_spans: HashMap<String, Option<Span>> = HashMap::new();
+
let mut from = from.into_iter();
+ let first = from.next().unwrap();
+
+ if let Some((name, span)) = extract_table_name(&first) {
+ alias_spans.entry(name).or_insert(span);
+ }
+
+ let mut left = LogicalPlanBuilder::from(
+ self.plan_table_with_joins(first, planner_context)?,
+ );
- let mut left = LogicalPlanBuilder::from({
- let input = from.next().unwrap();
- self.plan_table_with_joins(input, planner_context)?
- });
let old_outer_from_schema = {
let left_schema = Some(Arc::clone(left.schema()));
planner_context.set_outer_from_schema(left_schema)
};
for input in from {
- // Join `input` with the current result (`left`).
+ let current_name = extract_table_name(&input);
+
+ if let Some((ref name, ref span)) = current_name {
+ alias_spans.entry(name.clone()).or_insert(*span);
Review Comment:
Thanks I did not realize this. I've added two test cases that had error due to this and with multiple tables. I think it should be ok now
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Attach diagnostic information for duplicate table name error [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #20720: Attach diagnostic information for duplicate table name error
URL: https://github.com/apache/datafusion/pull/20720
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org