kosiew commented on code in PR #26202:
URL: https://github.com/apache/datafusion/pull/26202#discussion_r4347279102
##########
datafusion/sql/src/relation/mod.rs:
##########
@@ -90,7 +91,124 @@ 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![];
+ for relation in relations {
+ 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.
+ pub(crate) fn with_duplicate_diagnostic(
+ err: DataFusionError,
+ duplicates: &[DuplicateRelationName],
+ ) -> DataFusionError {
+ let diagnostic = match err.find_root() {
Review Comment:
[P2] An outer duplicate can shadow the diagnostic from the failing inner
scope. For `SELECT * FROM (SELECT * FROM person AS x, person AS x) AS n, j1 AS
x, j2 AS x`, `find_root()` matches the outer `x` and `with_diagnostic` wraps
the error again, so `err.diagnostic()` reports the outer aliases instead of the
conflicting inner relations. Please preserve the existing diagnostic or
associate enrichment with the failing scope, and add an inner/outer same-alias
regression, including the aliased parenthesized-JOIN variant.
##########
datafusion/sql/src/relation/mod.rs:
##########
@@ -90,7 +91,124 @@ 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![];
+ for relation in relations {
+ 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.
+ pub(crate) fn with_duplicate_diagnostic(
+ err: DataFusionError,
+ duplicates: &[DuplicateRelationName],
+ ) -> DataFusionError {
+ 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), .. }
+ | TableFactor::Derived { alias: Some(a), .. }
+ | TableFactor::Function { alias: Some(a), .. }
+ | TableFactor::UNNEST { alias: Some(a), .. }
+ | TableFactor::NestedJoin { alias: Some(a), .. } => {
Review Comment:
[P2] Unaliased `NestedJoin` factors expose their relation names to the
enclosing `FROM` scope, but this collector skips them. Both `SELECT * FROM
(person CROSS JOIN j1), person` and the reversed form produce no warning and a
duplicate `person.id` error without a diagnostic. Please traverse unaliased
nested joins, stop at alias and derived-query boundaries, avoid duplicate
reporting, and add comma-separated and explicit-JOIN regression cases.
##########
datafusion/sql/tests/cases/diagnostic.rs:
##########
@@ -791,3 +810,211 @@ fn test_window_function_alias_in_having() -> Result<()> {
assert_eq!(diag.span, Some(spans["a"]));
Ok(())
}
+
+#[test]
+fn test_duplicate_cte_name() -> Result<()> {
+ let query = "WITH /*a*/cte/*a*/ AS (SELECT 1 AS col), /*b*/cte/*b*/ AS
(SELECT 2 AS col) SELECT 1";
+ let spans = get_spans(query);
+ let diag = do_query(query);
+ assert_snapshot!(diag.message, @r#"WITH query name "cte" specified more
than once"#);
+ assert_eq!(diag.span, Some(spans["b"]));
+ assert_eq!(diag.notes.len(), 1);
+ assert_snapshot!(diag.notes[0].message, @"previously defined here");
+ assert_eq!(diag.notes[0].span, Some(spans["a"]));
+ Ok(())
+}
+
+#[test]
+fn test_duplicate_table_name() {
+ // The duplicated name in each query is marked `a` at its first occurrence
+ // and `b` at the repeated one.
+ // Cases that fail planning because the merged schema holds the same
+ // qualified field twice, as before the warning was added. Every other case
+ // plans successfully.
+ const FAILING: &[&str] = &[
+ "comma_basic_alias",
+ "comma_alias_not_first",
+ "comma_bare_table",
+ "comma_nested_join",
+ "explicit_join_bare_table",
+ ];
+ let cases: &[(&str, &str, &str)] = &[
+ // (label, query, duplicated name)
+ (
+ "comma_basic_alias",
+ "SELECT * FROM person /*a*/a/*a*/, person /*b*/a/*b*/",
+ "a",
+ ),
+ (
+ "comma_alias_not_first",
+ "SELECT * FROM person a, test_decimal /*a*/b/*a*/, person
/*b*/b/*b*/",
+ "b",
+ ),
+ (
+ "comma_bare_table",
+ "SELECT * FROM /*a*/person/*a*/, /*b*/person/*b*/",
+ "person",
+ ),
+ (
+ "comma_non_overlapping_columns",
+ "SELECT * FROM j1 AS /*a*/t/*a*/, j2 AS /*b*/t/*b*/",
+ "t",
+ ),
+ (
+ "comma_non_overlapping_three_tables",
+ "SELECT * FROM j1 AS x, j2 AS /*a*/t/*a*/, j3 AS y, j1 AS
/*b*/t/*b*/",
+ "t",
+ ),
+ (
+ "comma_derived_subquery",
+ "SELECT * FROM (SELECT 1) AS /*a*/t/*a*/, (SELECT 2) AS
/*b*/t/*b*/",
+ "t",
+ ),
+ (
+ "comma_table_and_derived",
+ "SELECT * FROM person AS /*a*/t/*a*/, (SELECT 1) AS /*b*/t/*b*/",
+ "t",
+ ),
+ (
+ "comma_derived_and_table",
+ "SELECT * FROM (SELECT 1) AS /*a*/t/*a*/, person AS /*b*/t/*b*/",
+ "t",
+ ),
+ (
+ "comma_nested_join",
+ "SELECT * FROM (person CROSS JOIN j1) AS /*a*/t/*a*/, (person
CROSS JOIN j2) AS /*b*/t/*b*/",
+ "t",
+ ),
+ (
+ "explicit_join_alias",
+ "SELECT 1 FROM j1 AS /*a*/t/*a*/ JOIN j2 AS /*b*/t/*b*/ ON true",
+ "t",
+ ),
+ (
+ "explicit_join_derived",
+ "SELECT 1 FROM (SELECT 1 AS a) AS /*a*/t/*a*/ JOIN (SELECT 2 AS b)
AS /*b*/t/*b*/ ON true",
+ "t",
+ ),
+ (
+ "comma_and_explicit_join",
+ "SELECT 1 FROM j1 AS /*a*/t/*a*/, j2 JOIN j3 AS /*b*/t/*b*/ ON
true",
+ "t",
+ ),
+ (
+ "explicit_join_bare_table",
+ "SELECT 1 FROM /*a*/person/*a*/ JOIN /*b*/person/*b*/ ON true",
+ "person",
+ ),
+ (
+ "explicit_cross_join",
+ "SELECT 1 FROM j1 AS /*a*/t/*a*/ CROSS JOIN j2 AS /*b*/t/*b*/",
+ "t",
+ ),
+ (
+ "explicit_left_semi_join",
+ "SELECT 1 FROM j1 AS /*a*/t/*a*/ LEFT SEMI JOIN j2 AS /*b*/t/*b*/
ON true",
+ "t",
+ ),
+ (
+ "explicit_left_join",
+ "SELECT 1 FROM j1 AS /*a*/t/*a*/ LEFT JOIN j2 AS /*b*/t/*b*/ ON
true",
+ "t",
+ ),
+ (
+ "explicit_three_way_join",
+ "SELECT 1 FROM j1 AS /*a*/x/*a*/ JOIN j2 AS y ON true JOIN j3 AS
/*b*/x/*b*/ ON true",
+ "x",
+ ),
+ (
+ "explicit_between_joins",
+ "SELECT 1 FROM j1 AS x JOIN j2 AS /*a*/y/*a*/ ON true JOIN j3 AS
/*b*/y/*b*/ ON true",
+ "y",
+ ),
+ (
+ "explicit_nested_join",
+ "SELECT 1 FROM (j1 CROSS JOIN j2) AS /*a*/t/*a*/ JOIN j3 AS
/*b*/t/*b*/ ON true",
+ "t",
+ ),
+ (
+ "inside_nested_join",
+ "SELECT 1 FROM (j1 AS /*a*/t/*a*/ CROSS JOIN j2 AS /*b*/t/*b*/) AS
n",
+ "t",
+ ),
+ (
+ "cross_entry_joins",
+ "SELECT 1 FROM j1 JOIN j2 AS /*a*/t/*a*/ ON true, j3 JOIN person
AS /*b*/t/*b*/ ON true",
+ "t",
+ ),
+ (
+ "case_insensitive",
+ "SELECT 1 FROM j1 AS /*a*/T/*a*/ JOIN j2 AS /*b*/t/*b*/ ON true",
+ "t",
+ ),
+ ];
+
+ for &(label, query, name) in cases {
+ let spans = get_spans(query);
+ let (result, warnings) = plan_query(query);
+ let message = format!("table name {name:?} specified more than once");
+ let check = |diag: &Diagnostic, kind: DiagnosticKind| {
+ assert_eq!(diag.kind, kind, "kind mismatch in {label}");
+ assert_eq!(diag.message, message, "message mismatch in {label}");
+ assert_eq!(diag.span, Some(spans["b"]), "span mismatch in
{label}");
+ assert_eq!(diag.notes.len(), 1, "expected 1 note in {label}");
+ assert_eq!(
+ diag.notes[0].message, "first defined here",
+ "note mismatch in {label}"
+ );
+ assert_eq!(
+ diag.notes[0].span,
+ Some(spans["a"]),
+ "note span mismatch in {label}"
+ );
+ };
+
+ // Planning outcome is unchanged by the duplicate; exactly one warning
+ // is always reported.
+ assert_eq!(warnings.len(), 1, "expected 1 warning in {label}");
+ check(&warnings[0], DiagnosticKind::Warning);
+
+ match (result, FAILING.contains(&label)) {
+ (Ok(()), false) => {}
+ (Err(err), true) => {
+ // The original error text is kept.
+ assert!(
+ err.strip_backtrace()
+ .contains("duplicate qualified field name"),
+ "unexpected error in {label}: {err}"
+ );
+ check(
+ err.diagnostic().expect("expected diagnostic"),
+ DiagnosticKind::Error,
+ );
+ }
+ (Ok(()), true) => panic!("expected {label} to fail planning"),
+ (Err(err), false) => panic!("expected {label} to plan, got:
{err}"),
+ }
+ }
+}
+
+#[test]
+fn test_duplicate_table_name_single_warning_in_multi_entry_from() {
Review Comment:
Non-blocking: could we add a three-occurrence case such as `SELECT 1 FROM
(SELECT 1 AS a) t, (SELECT 2 AS b) t, (SELECT 3 AS c) t`? Please assert exactly
two warnings, each pointing to its repeated alias and both referencing the
first occurrence. This would verify first-occurrence retention beyond the
existing two-occurrence tests.
##########
datafusion/sql/tests/cases/diagnostic.rs:
##########
@@ -791,3 +810,211 @@ fn test_window_function_alias_in_having() -> Result<()> {
assert_eq!(diag.span, Some(spans["a"]));
Ok(())
}
+
Review Comment:
Optional: consider testing duplicate CTEs with both alias-name AST spans
cleared. The original error text should remain, with an error diagnostic
lacking a primary span and no previous-definition note. This would cover the
optional-span fallback for programmatically constructed ASTs, which the current
parser-based tests do not exercise.
##########
datafusion/sql/src/relation/mod.rs:
##########
@@ -90,7 +91,124 @@ 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![];
+ for relation in relations {
+ 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 {
Review Comment:
Non-blocking policy question: should duplicate relation names eventually
fail planning regardless of column overlap? `SELECT 1 FROM (SELECT 1 AS a) t,
(SELECT 2 AS b) t` currently succeeds, while overlapping qualified columns
cause failure. I understand this PR intentionally preserves existing behavior,
so please confirm that compatibility policy. Stricter rejection could be
tracked separately, with appropriate scope, qualifier, and semi/anti-join
regression coverage.
--
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]
