Skip to content

Commit 048005d

Browse files
buraksennburaksenn
committed
detect duplicate table aliases in explicit JOINs and use full table reference for unaliased tables and refactor tests
1 parent db79b6d commit 048005d

4 files changed

Lines changed: 258 additions & 170 deletions

File tree

‎datafusion/sql/src/relation/join.rs‎

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,26 +16,54 @@
1616
// under the License.
1717

1818
use crate::planner::{ContextProvider, PlannerContext, SqlToRel};
19-
use datafusion_common::{Column, Result, not_impl_err, plan_datafusion_err};
19+
use datafusion_common::{
20+
Column, Diagnostic, Result, Span, not_impl_err, plan_datafusion_err, plan_err,
21+
};
2022
use datafusion_expr::{JoinType, LogicalPlan, LogicalPlanBuilder};
2123
use sqlparser::ast::{
2224
Join, JoinConstraint, JoinOperator, ObjectName, TableFactor, TableWithJoins,
2325
};
24-
use std::collections::HashSet;
26+
use std::collections::{HashMap, HashSet};
2527

2628
impl<S: ContextProvider> SqlToRel<'_, S> {
2729
pub(crate) fn plan_table_with_joins(
2830
&self,
2931
t: TableWithJoins,
3032
planner_context: &mut PlannerContext,
3133
) -> Result<LogicalPlan> {
32-
let mut left = if is_lateral(&t.relation) {
33-
self.create_relation_subquery(t.relation, planner_context)?
34+
let TableWithJoins { relation, joins } = t;
35+
36+
let mut alias_spans: HashMap<String, Option<Span>> = HashMap::new();
37+
38+
if let Some((name, span)) = self.extract_relation_name(&relation)? {
39+
alias_spans.insert(name, span);
40+
}
41+
42+
let mut left = if is_lateral(&relation) {
43+
self.create_relation_subquery(relation, planner_context)?
3444
} else {
35-
self.create_relation(t.relation, planner_context)?
45+
self.create_relation(relation, planner_context)?
3646
};
3747
let old_outer_from_schema = planner_context.outer_from_schema();
38-
for join in t.joins {
48+
for join in joins {
49+
if let Some((ref name, current_span)) =
50+
self.extract_relation_name(&join.relation)?
51+
{
52+
if let Some(prior_span) = alias_spans.get(name) {
53+
let mut diagnostic = Diagnostic::new_error(
54+
"duplicate table alias in FROM clause",
55+
current_span,
56+
);
57+
if let Some(span) = *prior_span {
58+
diagnostic =
59+
diagnostic.with_note("first defined here", Some(span));
60+
}
61+
return plan_err!("duplicate table alias in FROM clause")
62+
.map_err(|e| e.with_diagnostic(diagnostic));
63+
}
64+
alias_spans.insert(name.clone(), current_span);
65+
}
66+
3967
planner_context.extend_outer_from_schema(left.schema())?;
4068
left = self.parse_relation_join(left, join, planner_context)?;
4169
}

‎datafusion/sql/src/relation/mod.rs‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,31 @@ impl<'a, 'b, S: ContextProvider> RelationPlannerContext
8080
}
8181

8282
impl<S: ContextProvider> SqlToRel<'_, S> {
83+
pub(crate) fn extract_relation_name(
84+
&self,
85+
relation: &TableFactor,
86+
) -> Result<Option<(String, Option<Span>)>> {
87+
match relation {
88+
TableFactor::Table { alias: Some(a), .. }
89+
| TableFactor::Derived { alias: Some(a), .. }
90+
| TableFactor::Function { alias: Some(a), .. }
91+
| TableFactor::UNNEST { alias: Some(a), .. }
92+
| TableFactor::NestedJoin { alias: Some(a), .. } => {
93+
let span = Span::try_from_sqlparser_span(a.name.span);
94+
let name = self.ident_normalizer.normalize(a.name.clone());
95+
Ok(Some((name, span)))
96+
}
97+
TableFactor::Table {
98+
name, alias: None, ..
99+
} => {
100+
let span = Span::try_from_sqlparser_span(relation.span());
101+
let table_ref = self.object_name_to_table_reference(name.clone())?;
102+
Ok(Some((table_ref.to_string(), span)))
103+
}
104+
_ => Ok(None),
105+
}
106+
}
107+
83108
/// Create a `LogicalPlan` that scans the named relation.
84109
///
85110
/// First tries any registered extension planners. If no extension handles

‎datafusion/sql/src/select.rs‎

Lines changed: 25 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -52,9 +52,7 @@ use sqlparser::ast::{
5252
SelectItemQualifiedWildcardKind, WildcardAdditionalOptions, WindowType,
5353
visit_expressions_mut,
5454
};
55-
use sqlparser::ast::{
56-
NamedWindowDefinition, Select, SelectItem, Spanned, TableFactor, TableWithJoins,
57-
};
55+
use sqlparser::ast::{NamedWindowDefinition, Select, SelectItem, TableWithJoins};
5856

5957
/// Result of the `aggregate` function, containing the aggregate plan and
6058
/// rewritten expressions that reference the aggregate output columns.
@@ -694,44 +692,21 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
694692
self.plan_table_with_joins(input, planner_context)
695693
}
696694
_ => {
697-
let extract_table_name =
698-
|t: &TableWithJoins| -> Option<(String, Option<Span>)> {
699-
match &t.relation {
700-
TableFactor::Table { alias: Some(a), .. }
701-
| TableFactor::Derived { alias: Some(a), .. }
702-
| TableFactor::Function { alias: Some(a), .. }
703-
| TableFactor::UNNEST { alias: Some(a), .. }
704-
| TableFactor::NestedJoin { alias: Some(a), .. } => {
705-
let span = Span::try_from_sqlparser_span(a.name.span);
706-
let name =
707-
self.ident_normalizer.normalize(a.name.clone());
708-
Some((name, span))
709-
}
710-
TableFactor::Table {
711-
name, alias: None, ..
712-
} => {
713-
let span =
714-
Span::try_from_sqlparser_span(t.relation.span());
715-
let table_name = name
716-
.0
717-
.iter()
718-
.filter_map(|p| p.as_ident())
719-
.map(|id| self.ident_normalizer.normalize(id.clone()))
720-
.next_back()?;
721-
Some((table_name, span))
722-
}
723-
_ => None,
724-
}
725-
};
726-
727695
let mut alias_spans: HashMap<String, Option<Span>> = HashMap::new();
728696

729697
let mut from = from.into_iter();
730698
let first = from.next().unwrap();
731699

732-
if let Some((name, span)) = extract_table_name(&first) {
700+
if let Some((name, span)) = self.extract_relation_name(&first.relation)? {
733701
alias_spans.entry(name).or_insert(span);
734702
}
703+
for join in &first.joins {
704+
if let Some((name, span)) =
705+
self.extract_relation_name(&join.relation)?
706+
{
707+
alias_spans.entry(name).or_insert(span);
708+
}
709+
}
735710

736711
let mut left = LogicalPlanBuilder::from(
737712
self.plan_table_with_joins(first, planner_context)?,
@@ -742,13 +717,21 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
742717
planner_context.set_outer_from_schema(left_schema)
743718
};
744719
for input in from {
745-
let current_name = extract_table_name(&input);
720+
let mut current_names = Vec::new();
721+
if let Some(pair) = self.extract_relation_name(&input.relation)? {
722+
current_names.push(pair);
723+
}
724+
for join in &input.joins {
725+
if let Some(pair) = self.extract_relation_name(&join.relation)? {
726+
current_names.push(pair);
727+
}
728+
}
746729

747-
if let Some((ref name, current_span)) = current_name {
748-
if let Some(prior_span) = alias_spans.get(name) {
730+
for (name, current_span) in &current_names {
731+
if let Some(prior_span) = alias_spans.get(name.as_str()) {
749732
let mut diagnostic = Diagnostic::new_error(
750733
"duplicate table alias in FROM clause",
751-
current_span,
734+
*current_span,
752735
);
753736
if let Some(span) = *prior_span {
754737
diagnostic = diagnostic
@@ -757,7 +740,10 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
757740
return plan_err!("duplicate table alias in FROM clause")
758741
.map_err(|e| e.with_diagnostic(diagnostic));
759742
}
760-
alias_spans.insert(name.clone(), current_span);
743+
}
744+
745+
for (name, span) in current_names {
746+
alias_spans.insert(name, span);
761747
}
762748

763749
let right = self.plan_table_with_joins(input, planner_context)?;

0 commit comments

Comments
 (0)