Skip to content
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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jun 5, 2024
Merged

Handle EmptyRelation during SQL unparsing #10803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4d914ea
handle empty relation for sql unparsing
goldmedal Jun 5, 2024
9f4876f
revert the doc test change
goldmedal Jun 5, 2024
c28ef9a
remove dbg macro
goldmedal Jun 5, 2024
7289b33
cargo fmt
goldmedal Jun 5, 2024
73f37ca
improve the error handling
goldmedal Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 22 additions & 15 deletions datafusion/sql/src/unparser/ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ impl SelectBuilder {
from: self
.from
.iter()
.map(|b| b.build())
.filter_map(|b| b.build().transpose())
.collect::<Result<Vec<_>, BuilderError>>()?,
lateral_views: self.lateral_views.clone(),
selection: self.selection.clone(),
Expand Down Expand Up @@ -314,18 +314,17 @@ impl TableWithJoinsBuilder {
new
}

pub fn build(&self) -> Result<ast::TableWithJoins, BuilderError> {
Ok(ast::TableWithJoins {
relation: match self.relation {
Some(ref value) => value.build()?,
None => {
return Result::Err(Into::into(UninitializedFieldError::from(
"relation",
)))
}
pub fn build(&self) -> Result<Option<ast::TableWithJoins>, BuilderError> {
match self.relation {
Some(ref value) => match value.build()? {
Some(relation) => Ok(Some(ast::TableWithJoins {
relation,
joins: self.joins.clone(),
})),
None => Ok(None),
},
joins: self.joins.clone(),
})
None => Err(Into::into(UninitializedFieldError::from("relation"))),
}
}
fn create_empty() -> Self {
Self {
Expand All @@ -350,6 +349,7 @@ pub(super) struct RelationBuilder {
enum TableFactorBuilder {
Table(TableRelationBuilder),
Derived(DerivedRelationBuilder),
Empty,
}

#[allow(dead_code)]
Expand All @@ -367,6 +367,11 @@ impl RelationBuilder {
new.relation = Option::Some(TableFactorBuilder::Derived(value));
new
}
pub fn empty(&mut self) -> &mut Self {
let new = self;
new.relation = Some(TableFactorBuilder::Empty);
new
Comment on lines +371 to +373
Copy link
Contributor

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)

Suggested change
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

}
pub fn alias(&mut self, value: Option<ast::TableAlias>) -> &mut Self {
let new = self;
match new.relation {
Expand All @@ -376,14 +381,16 @@ impl RelationBuilder {
Some(TableFactorBuilder::Derived(ref mut rel_builder)) => {
rel_builder.alias = value;
}
Some(TableFactorBuilder::Empty) => (),
None => (),
}
new
}
pub fn build(&self) -> Result<ast::TableFactor, BuilderError> {
pub fn build(&self) -> Result<Option<ast::TableFactor>, BuilderError> {
Ok(match self.relation {
Some(TableFactorBuilder::Table(ref value)) => value.build()?,
Some(TableFactorBuilder::Derived(ref value)) => value.build()?,
Some(TableFactorBuilder::Table(ref value)) => Some(value.build()?),
Some(TableFactorBuilder::Derived(ref value)) => Some(value.build()?),
Some(TableFactorBuilder::Empty) => None,
None => {
return Result::Err(Into::into(UninitializedFieldError::from("relation")))
}
Expand Down
14 changes: 12 additions & 2 deletions datafusion/sql/src/unparser/plan.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,10 @@ impl Unparser<'_> {
)?;

let ast_join = ast::Join {
Copy link
Contributor

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),
                };

relation: right_relation.build()?,
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),
};
Expand Down Expand Up @@ -417,7 +420,10 @@ impl Unparser<'_> {
)?;

let ast_join = ast::Join {
relation: right_relation.build()?,
relation: match right_relation.build()? {
Some(relation) => relation,
None => return internal_err!("Failed to build right relation"),
},
join_operator: self.join_operator_to_sql(
JoinType::Inner,
ast::JoinConstraint::On(ast::Expr::Value(ast::Value::Boolean(
Expand Down Expand Up @@ -483,6 +489,10 @@ impl Unparser<'_> {
relation,
)
}
LogicalPlan::EmptyRelation(_) => {
relation.empty();
Ok(())
}
LogicalPlan::Extension(_) => not_impl_err!("Unsupported operator: {plan:?}"),
_ => not_impl_err!("Unsupported operator: {plan:?}"),
}
Expand Down
5 changes: 5 additions & 0 deletions datafusion/sql/tests/cases/plan_to_sql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ fn roundtrip_expr() {
#[test]
fn roundtrip_statement() -> Result<()> {
let tests: Vec<&str> = vec![
"select 1;",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

"select 1 limit 0;",
"select ta.j1_id from j1 ta join (select 1 as j1_id) tb on ta.j1_id = tb.j1_id;",
"select ta.j1_id from j1 ta join (select 1 as j1_id) tb on ta.j1_id = tb.j1_id where ta.j1_id > 1;",
"select ta.j1_id from (select 1 as j1_id) ta;",
"select ta.j1_id from j1 ta;",
"select ta.j1_id from j1 ta order by ta.j1_id;",
"select * from j1 ta order by ta.j1_id, ta.j1_string desc;",
Expand Down