haochunchang commented on code in PR #26202:
URL: https://github.com/apache/datafusion/pull/26202#discussion_r4388849771
##########
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:
@kosiew Very appreciate your review with clear feedback!
I will add `test_duplicate_table_name_inner_scope_diagnostic_not_shadowed`
in `datafusion/sql/tests/cases/ diagnostic.rs` with asserted inner
diagnostics for 2 queries:
1. the plain nested subquery
2. the aliased parenthesized CROSS JOIN variant
The guard is adding a
```rust
if err.diagnostic().is_some() {
return err;
}
```
before `err.find_root()` so that it won't attach diagnostic into the outer
scope.
--
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]
