goldmedal commented on code in PR #11636:
URL: https://github.com/apache/datafusion/pull/11636#discussion_r1690064067
##########
datafusion/sql/src/unparser/plan.rs:
##########
@@ -582,9 +552,62 @@ impl Unparser<'_> {
}
}
+ /// Convert the components of a USING clause to the USING AST
+ fn join_using_to_sql(&self, join_conditions: &[(Expr, Expr)]) ->
Result<ast::JoinConstraint> {
+ let mut idents = Vec::with_capacity(join_conditions.len());
+ for (left, right) in join_conditions {
+ match (left, right) {
+ (Expr::Column(Column { relation: _, name: left_name }),
Expr::Column(Column { relation: _, name: right_name })) => {
+ if left_name != right_name {
+ // USING is only valid when the column names are the
+ // same, so they should never be different
+ return not_impl_err!("Unsupported USING with different
column names");
+ }
+
idents.push(self.new_ident_quoted_if_needs(left_name.to_string()));
+ },
+ // USING is only valid with column names; arbitrary expressions
+ // are not allowed
+ _ => return not_impl_err!("Unsupported USING with non-column
expressions"),
+ }
+ }
+ Ok(ast::JoinConstraint::Using(idents))
+ }
+
+ /// Convert a join constraint and associated conditions and filter to a
SQL AST node
+ fn join_constraint_to_sql(&self, constraint: JoinConstraint, conditions:
&[(Expr, Expr)], filter: Option<&Expr>) -> Result<ast::JoinConstraint> {
+ match (constraint, conditions, filter) {
+ // No constraints
+ (JoinConstraint::On, [], None) => Ok(ast::JoinConstraint::None),
+ // Only equi-join conditions
+ (JoinConstraint::On, conditions, None) => {
+ let expr = self.join_conditions_to_sql(conditions,
ast::BinaryOperator::Eq)?;
+ match expr {
+ Some(expr) => Ok(ast::JoinConstraint::On(expr)),
+ None => Ok(ast::JoinConstraint::None),
+ }
+ },
+ // More complex filter with non-equi-join conditions; so we combine
+ // all conditions into a single AST Expr
+ (JoinConstraint::On, conditions, Some(filter)) => {
+ let filter_expr = self.expr_to_sql(filter)?;
+ let expr = self.join_conditions_to_sql(conditions,
ast::BinaryOperator::Eq)?;
+ match expr {
+ Some(expr) => {
+ let join_expr = self.and_op_to_sql(filter_expr, expr);
+ Ok(ast::JoinConstraint::On(join_expr))
+ },
+ None => Ok(ast::JoinConstraint::On(filter_expr)),
+ }
+ },
+
+ (JoinConstraint::Using, conditions, None) =>
self.join_using_to_sql(conditions),
+ (JoinConstraint::Using, _, Some(_)) => not_impl_err!("Unsupported
USING with filter"),
+ }
+ }
+
fn join_conditions_to_sql(
&self,
- join_conditions: &Vec<(Expr, Expr)>,
+ join_conditions: &[(Expr, Expr)],
Review Comment:
👍
##########
datafusion/sql/src/unparser/plan.rs:
##########
@@ -582,9 +558,89 @@ impl Unparser<'_> {
}
}
+ /// Convert the components of a USING clause to the USING AST
+ fn join_using_to_sql(
+ &self,
+ join_conditions: &[(Expr, Expr)],
+ ) -> Result<ast::JoinConstraint> {
+ let mut idents = Vec::with_capacity(join_conditions.len());
+ for (left, right) in join_conditions {
+ match (left, right) {
+ (
+ Expr::Column(Column {
+ relation: _,
+ name: left_name,
+ }),
+ Expr::Column(Column {
+ relation: _,
+ name: right_name,
+ }),
+ ) => {
+ if left_name != right_name {
+ // USING is only valid when the column names are the
+ // same, so they should never be different
+ return not_impl_err!(
+ "Unsupported USING with different column names"
+ );
Review Comment:
IMO, `not_impl_err` is for some todo items. If this case never happens, I
prefer to use `internal_err` here.
```suggestion
return internal_err!(
"Unsupported USING with different column names"
);
```
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
