-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle EmptyRelation during SQL unparsing #10803
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @goldmedal this looks good to me 🚀. I have one minor suggestion to avoid a panic.
datafusion/sql/src/unparser/ast.rs
Outdated
@@ -242,8 +242,8 @@ impl SelectBuilder { | |||
from: self | |||
.from | |||
.iter() | |||
.map(|b| b.build()) | |||
.collect::<Result<Vec<_>, BuilderError>>()?, | |||
.filter_map(|b| b.build().ok().unwrap()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change will result in a panic on a UninitializedFieldError
due to the unwrap
. E.g. if self.relation is None
rather than Some(None)
. I think we could avoid this by using Result::transpose rather than .ok().unwrap()
here and then continuing to collect::<Result<T>, _>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @devinjdangelo. Nice idea. I've learned more about Rust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me -- thank you @goldmedal and @devinjdangelo
@@ -78,6 +78,11 @@ fn roundtrip_expr() { | |||
#[test] | |||
fn roundtrip_statement() -> Result<()> { | |||
let tests: Vec<&str> = vec![ | |||
"select 1;", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
let new = self; | ||
new.relation = Some(TableFactorBuilder::Empty); | ||
new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could also do this (which is more concise)
let new = self; | |
new.relation = Some(TableFactorBuilder::Empty); | |
new | |
self.relation = Some(TableFactorBuilder::Empty); | |
new |
Though I see it follows the same pattern as the code above so if we are going to change any of the we should change them all
@@ -389,7 +389,10 @@ impl Unparser<'_> { | |||
)?; | |||
|
|||
let ast_join = ast::Join { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is totally fine, but I figured I would point out another way to express this logic that you might prefer in future PRs
Instead of
let ast_join = ast::Join {
relation: match right_relation.build()? {
Some(relation) => relation,
None => return internal_err!("Failed to build right relation"),
},
join_operator: self
.join_operator_to_sql(join.join_type, join_constraint),
};
You can do something like this with a let .. else
to match or return
let Some(relation) = match right_relation.build()? else {
return internal_err!("Failed to build right relation"),
};
let ast_join = ast::Join {
relation,
join_operator: self
.join_operator_to_sql(join.join_type, join_constraint),
};
Thanks @alamb @devinjdangelo |
* handle empty relation for sql unparsing * revert the doc test change * remove dbg macro * cargo fmt * improve the error handling
Which issue does this PR close?
Closes #10799 .
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
No