You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "wackywendell (via GitHub)" <gi...@apache.org> on 2024/07/24 15:36:38 UTC
[PR] Add support for USING to SQL unparser [datafusion]
wackywendell opened a new pull request, #11636:
URL: https://github.com/apache/datafusion/pull/11636
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #10652.
## Are these changes tested?
Yes - I added a couple example cases to the existing tests.
## Are there any user-facing changes?
Only the above change.
## Other
👋 Hello! This is my first contribution to apache/datafusion; all feedback welcome! I expect there will be a bit of learning on my side as to how this community works. Thanks for any help there you can provide!
Also - this passes `cargo test` locally, but if someone could trigger CI please, that would be appreciated 🙇
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for USING to SQL unparser [datafusion]
Posted by "wackywendell (via GitHub)" <gi...@apache.org>.
wackywendell commented on code in PR #11636:
URL: https://github.com/apache/datafusion/pull/11636#discussion_r1690036503
##########
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");
Review Comment:
Should these be `not_impl_err`? If I understand correctly, this is an invalid state, so arguably its something like an `Internal` error... same for below.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for USING to SQL unparser [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #11636:
URL: https://github.com/apache/datafusion/pull/11636#issuecomment-2248455854
> Speaking of which - should I squash/rebase? By default I tend not to do that on public PRs/branches, as it can be confusing for reviews.
Unless you think that rebase or squash makes the review easier, I don't think you need to do it. We don't have any rules requiring you to do so.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for USING to SQL unparser [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for USING to SQL unparser [datafusion]
Posted by "wackywendell (via GitHub)" <gi...@apache.org>.
wackywendell commented on PR #11636:
URL: https://github.com/apache/datafusion/pull/11636#issuecomment-2248748056
Pushed an update - to get the fallback from `USING` to `ON` to work and be reasonably succinct, I refactored a bit. I think it will need another review now - thanks so much!
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for USING to SQL unparser [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11636:
URL: https://github.com/apache/datafusion/pull/11636#issuecomment-2252948911
🚀
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for USING to SQL unparser [datafusion]
Posted by "wackywendell (via GitHub)" <gi...@apache.org>.
wackywendell commented on PR #11636:
URL: https://github.com/apache/datafusion/pull/11636#issuecomment-2248578170
Thanks all for your super-quick reviews! Working on an update now.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for USING to SQL unparser [datafusion]
Posted by "wackywendell (via GitHub)" <gi...@apache.org>.
wackywendell commented on code in PR #11636:
URL: https://github.com/apache/datafusion/pull/11636#discussion_r1690138973
##########
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:
That can happen with `ON`, but it can't happen with `USING`, right? This function is only for `USING`.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for USING to SQL unparser [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11636:
URL: https://github.com/apache/datafusion/pull/11636#discussion_r1691097502
##########
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:
I agree -- this is a nice refactoring and very nicely commented code.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for USING to SQL unparser [datafusion]
Posted by "wackywendell (via GitHub)" <gi...@apache.org>.
wackywendell commented on PR #11636:
URL: https://github.com/apache/datafusion/pull/11636#issuecomment-2248376015
^ I installed the `pre-commit.sh` and turned on clippy and `cargo fmt` in my editor; caught the above in 11324895d04.
Speaking of which - should I squash/rebase? By default I tend not to do that on public PRs/branches, as it can be confusing for reviews.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for USING to SQL unparser [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11636:
URL: https://github.com/apache/datafusion/pull/11636#issuecomment-2248671711
> Unless you think that rebase or squash makes the review easier, I don't think you need to do it. We don't have any rules requiring you to do so.
Agreed -- the final merge into main is done with squash (so the entire PR turns into a single commit). I typically find it easier to review PRs where subsequent changes are made with new commits (rather than rebases) so it is more clear what changed
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for USING to SQL unparser [datafusion]
Posted by "wackywendell (via GitHub)" <gi...@apache.org>.
wackywendell commented on code in PR #11636:
URL: https://github.com/apache/datafusion/pull/11636#discussion_r1690203149
##########
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:
So I thought about this some more - these two cases should be impossible for a `LogicalPlan` made from SQL (you can't do `JOIN … USING (t1.a = t1.b)`, but could be created by a user directly creating a `LogicalPlan::Join(Join { … })`.
Given that, fallback makes sense, and I'll add a comment to explain.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for USING to SQL unparser [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11636:
URL: https://github.com/apache/datafusion/pull/11636#discussion_r1690124537
##########
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,
+ }),
+ ) => {
Review Comment:
Rather than returning "not implemented" it would be nice to just fall back to the existing code
I think you could do it with a match guard like
```suggestion
) if left_name == right_name => {
```
##########
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:
I think this case can happen -- like `JOIN ON (t1.a = t2.b)`
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add support for USING to SQL unparser [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #11636:
URL: https://github.com/apache/datafusion/pull/11636
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org