Add support for USING to SQL unparser #11636
Conversation
datafusion/sql/src/unparser/plan.rs
Outdated
| 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"); |
There was a problem hiding this 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.
|
^ I installed the 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. |
There was a problem hiding this comment.
Thanks @wackywendell for working on this. Overall LGTM.
However, I can't help you to trigger the CI. Could @alamb or @jayzhan211 help to trigger it?
| fn join_conditions_to_sql( | ||
| &self, | ||
| join_conditions: &Vec<(Expr, Expr)>, | ||
| join_conditions: &[(Expr, Expr)], |
There was a problem hiding this comment.
I agree -- this is a nice refactoring and very nicely commented code.
datafusion/sql/src/unparser/plan.rs
Outdated
| return not_impl_err!( | ||
| "Unsupported USING with different column names" | ||
| ); |
There was a problem hiding this comment.
IMO, not_impl_err is for some todo items. If this case never happens, I prefer to use internal_err here.
| return not_impl_err!( | |
| "Unsupported USING with different column names" | |
| ); | |
| return internal_err!( | |
| "Unsupported USING with different column names" | |
| ); |
There was a problem hiding this comment.
I think this case can happen -- like JOIN ON (t1.a = t2.b)
There was a problem hiding this comment.
That can happen with ON, but it can't happen with USING, right? This function is only for USING.
There was a problem hiding this 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.
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. |
There was a problem hiding this comment.
Thanks @wackywendell and @goldmedal -- this is a great first PR. I left some comments
datafusion/sql/src/unparser/plan.rs
Outdated
| return not_impl_err!( | ||
| "Unsupported USING with different column names" | ||
| ); |
There was a problem hiding this comment.
I think this case can happen -- like JOIN ON (t1.a = t2.b)
datafusion/sql/src/unparser/plan.rs
Outdated
| relation: _, | ||
| name: right_name, | ||
| }), | ||
| ) => { |
There was a problem hiding this 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
| ) => { | |
| ) if left_name == right_name => { |
|
Thanks all for your super-quick reviews! Working on an update now. |
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 |
When the conditions and filters in the LogicalPlan are not in a form compatible with USING, we can instead use ON - so we do.
|
Pushed an update - to get the fallback from |
| fn join_conditions_to_sql( | ||
| &self, | ||
| join_conditions: &Vec<(Expr, Expr)>, | ||
| join_conditions: &[(Expr, Expr)], |
There was a problem hiding this comment.
I agree -- this is a nice refactoring and very nicely commented code.
|
🚀 |
Which issue does this PR close?
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 listed change -
LogicalPlans withJoinConstraint::Usingcan now successfully be translated back to SQL.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 testlocally, but if someone could trigger CI please, that would be appreciated 🙇