Phoenix500526 commented on code in PR #26202:
URL: https://github.com/apache/datafusion/pull/26202#discussion_r4431072971
##########
datafusion/sql/src/relation/mod.rs:
##########
@@ -77,7 +96,135 @@ impl<S: ContextProvider> RelationPlannerContext for
SqlToRelRelationContext<'_,
}
}
+/// A relation name used more than once in a `FROM` scope.
+pub(crate) struct DuplicateRelationName {
+ qualifier: TableReference,
+ /// Span of the first relation with this name, if known.
+ first_span: Option<Span>,
+ /// Span of the repeated relation.
+ span: Option<Span>,
+}
+
+impl DuplicateRelationName {
+ fn diagnostic(&self, new: fn(String, Option<Span>) -> Diagnostic) ->
Diagnostic {
+ let msg = format!("table name \"{}\" specified more than once",
self.qualifier);
+ let diagnostic = new(msg, self.span);
+ match self.first_span {
+ Some(first) => diagnostic.with_note("first defined here",
Some(first)),
+ None => diagnostic,
+ }
+ }
+}
+
impl<S: ContextProvider> SqlToRel<'_, S> {
+ /// Finds relations in the same `FROM` scope that are visible under the
+ /// same name. This never fails planning: callers report each result as a
+ /// warning and, if planning then fails on the duplicate qualifier, attach
+ /// it to the error (see [`Self::with_duplicate_diagnostic`]).
+ pub(crate) fn find_duplicate_relation_names<'a>(
+ &self,
+ relations: impl IntoIterator<Item = &'a TableFactor>,
+ ) -> Vec<DuplicateRelationName> {
+ let mut seen: HashMap<TableReference, Option<Span>> = HashMap::new();
+ let mut duplicates = vec![];
+ let mut visible = vec![];
+ for relation in relations {
+ visible_relations(relation, &mut visible);
+ }
+ for relation in visible {
+ let Some((qualifier, span)) = self.extract_relation_name(relation)
else {
+ continue;
+ };
+ if let Some(first_span) = seen.get(&qualifier) {
+ duplicates.push(DuplicateRelationName {
+ qualifier,
+ first_span: *first_span,
+ span,
+ });
+ } else {
+ seen.insert(qualifier, span);
+ }
+ }
+ duplicates
+ }
+
+ /// Adds one warning per duplicate relation name.
+ pub(crate) fn warn_duplicate_relation_names(
+ &self,
+ duplicates: &[DuplicateRelationName],
+ ) {
+ for duplicate in duplicates {
+ self.add_warning(duplicate.diagnostic(Diagnostic::new_warning));
+ }
+ }
+
+ /// If `err` is a duplicate qualified field error for one of `duplicates`,
+ /// attaches a [`Diagnostic`] pointing at both relations. The error message
+ /// is unchanged.
+ ///
+ /// An error that already carries a diagnostic is returned as is: the
+ /// innermost failing scope attaches first, so an enclosing scope that
+ /// repeats the same name must not replace it.
+ pub(crate) fn with_duplicate_diagnostic(
+ err: DataFusionError,
+ duplicates: &[DuplicateRelationName],
+ ) -> DataFusionError {
+ if err.diagnostic().is_some() {
+ return err;
+ }
+ let diagnostic = match err.find_root() {
+ DataFusionError::SchemaError(schema_err, _) => match &**schema_err
{
+ SchemaError::DuplicateQualifiedField { qualifier, .. } =>
duplicates
+ .iter()
+ .find(|d| d.qualifier == **qualifier)
+ .map(|d| d.diagnostic(Diagnostic::new_error)),
+ _ => None,
+ },
+ _ => None,
+ };
+ match diagnostic {
+ Some(diagnostic) => err.with_diagnostic(diagnostic),
+ None => err,
+ }
+ }
+
+ /// Returns the qualified name a relation is visible under in its `FROM`
+ /// scope (matching the qualifier of the planned relation), and the span of
+ /// that name. Returns `None` when the name cannot be resolved here, so
+ /// that planning reports the real error.
+ fn extract_relation_name(
+ &self,
+ relation: &TableFactor,
+ ) -> Option<(TableReference, Option<Span>)> {
+ match relation {
+ TableFactor::Table { alias: Some(a), .. }
Review Comment:
Could we also handle `TableFactor::Function { alias: None, .. }` here?
Unaliased `LATERAL` table functions fall through to `None`, so repeated calls
produce neither a duplicate-name warning nor a Diagnostic on the resulting
duplicate-field error.
##########
datafusion/sql/src/cte.rs:
##########
Review Comment:
Could we use insert_cte_with_span here? The duplicate-name error highlights
the second cte but omits “previously defined here” because insert_cte stores no
span:
```SQL
WITH cte AS MATERIALIZED (SELECT 1 AS col),
cte AS (SELECT 2 AS col)
SELECT 1;
```
The temporary work-table registration in recursive_cte also needs the name
span passed through:
```SQL
WITH RECURSIVE cte AS (
SELECT 1 AS col
UNION ALL (
WITH cte AS (SELECT 2 AS col)
SELECT col FROM cte
)
)
SELECT * FROM cte;
```
I reproduced the missing note in both cases.
##########
datafusion/sql/src/relation/mod.rs:
##########
@@ -77,7 +96,135 @@ impl<S: ContextProvider> RelationPlannerContext for
SqlToRelRelationContext<'_,
}
}
+/// A relation name used more than once in a `FROM` scope.
+pub(crate) struct DuplicateRelationName {
+ qualifier: TableReference,
+ /// Span of the first relation with this name, if known.
+ first_span: Option<Span>,
+ /// Span of the repeated relation.
+ span: Option<Span>,
+}
+
+impl DuplicateRelationName {
+ fn diagnostic(&self, new: fn(String, Option<Span>) -> Diagnostic) ->
Diagnostic {
+ let msg = format!("table name \"{}\" specified more than once",
self.qualifier);
+ let diagnostic = new(msg, self.span);
+ match self.first_span {
+ Some(first) => diagnostic.with_note("first defined here",
Some(first)),
+ None => diagnostic,
+ }
+ }
+}
+
impl<S: ContextProvider> SqlToRel<'_, S> {
+ /// Finds relations in the same `FROM` scope that are visible under the
+ /// same name. This never fails planning: callers report each result as a
+ /// warning and, if planning then fails on the duplicate qualifier, attach
+ /// it to the error (see [`Self::with_duplicate_diagnostic`]).
+ pub(crate) fn find_duplicate_relation_names<'a>(
+ &self,
+ relations: impl IntoIterator<Item = &'a TableFactor>,
+ ) -> Vec<DuplicateRelationName> {
+ let mut seen: HashMap<TableReference, Option<Span>> = HashMap::new();
+ let mut duplicates = vec![];
+ let mut visible = vec![];
+ for relation in relations {
+ visible_relations(relation, &mut visible);
+ }
+ for relation in visible {
+ let Some((qualifier, span)) = self.extract_relation_name(relation)
else {
+ continue;
+ };
+ if let Some(first_span) = seen.get(&qualifier) {
+ duplicates.push(DuplicateRelationName {
+ qualifier,
+ first_span: *first_span,
+ span,
+ });
+ } else {
+ seen.insert(qualifier, span);
+ }
+ }
+ duplicates
+ }
+
+ /// Adds one warning per duplicate relation name.
+ pub(crate) fn warn_duplicate_relation_names(
+ &self,
+ duplicates: &[DuplicateRelationName],
+ ) {
+ for duplicate in duplicates {
+ self.add_warning(duplicate.diagnostic(Diagnostic::new_warning));
+ }
+ }
+
+ /// If `err` is a duplicate qualified field error for one of `duplicates`,
+ /// attaches a [`Diagnostic`] pointing at both relations. The error message
+ /// is unchanged.
+ ///
+ /// An error that already carries a diagnostic is returned as is: the
+ /// innermost failing scope attaches first, so an enclosing scope that
+ /// repeats the same name must not replace it.
+ pub(crate) fn with_duplicate_diagnostic(
+ err: DataFusionError,
+ duplicates: &[DuplicateRelationName],
+ ) -> DataFusionError {
+ if err.diagnostic().is_some() {
+ return err;
+ }
+ let diagnostic = match err.find_root() {
+ DataFusionError::SchemaError(schema_err, _) => match &**schema_err
{
+ SchemaError::DuplicateQualifiedField { qualifier, .. } =>
duplicates
+ .iter()
+ .find(|d| d.qualifier == **qualifier)
Review Comment:
Could we associate the diagnostic with the relation that triggered the field
conflict, rather than selecting the first duplicate with the same qualifier?
For example:
```sql
SELECT *
FROM (SELECT 1 AS a) AS t,
(SELECT 2 AS b) AS t,
(SELECT 3 AS a) AS t;
```
The first two relations plan successfully together because their columns do
not overlap. Adding the third relation produces:
```text
Schema contains duplicate qualified field name t.a
```
However, the attached diagnostic points to the **second** `t`. Rendering its
spans against the SQL gives:
```text
error: table name "t" specified more than once
1 | SELECT *
2 | FROM (SELECT 1 AS a) AS t,
| - first defined here
3 | (SELECT 2 AS b) AS t,
| ^
4 | (SELECT 3 AS a) AS t;
```
I would expect it to highlight the **third** `t`, since the conflicting
`t.a` columns come from the first and third relations:
```text
error: table name "t" specified more than once
1 | SELECT *
2 | FROM (SELECT 1 AS a) AS t,
| - first defined here
3 | (SELECT 2 AS b) AS t,
4 | (SELECT 3 AS a) AS t;
| ^
```
I reproduced this locally. The two warning diagnostics point to the second
and third occurrences as expected, but this `find` selects the second
occurrence when attaching the error diagnostic.
--
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]
