Only unnest source for EmptyRelation
#15159
Conversation
datafusion/sql/src/unparser/plan.rs
Outdated
| if matches!( | ||
| projection.input.as_ref(), | ||
| LogicalPlan::EmptyRelation(_) |
There was a problem hiding this comment.
It's better to add some comments to explain why we match EmptyRelation.
There was a problem hiding this comment.
Thank you, added in 28f5301
There was a problem hiding this comment.
Had to remove this new test because now (even in main) it produces SELECT * FROM (SELECT [1, 2, 3] AS c) CROSS JOIN UNNEST(c). Feels like this should be discussed separately
There was a problem hiding this comment.
I don't understand this comment -- doesn't this PR add a new test?
There was a problem hiding this comment.
Sorry -- before I added one more test, but then I have to remove it due to ef4a79d - I think we can discuss in that issue and work on it further
There was a problem hiding this comment.
I don't understand this comment -- doesn't this PR add a new test?
datafusion/sql/src/unparser/plan.rs
Outdated
| @@ -377,8 +377,23 @@ impl Unparser<'_> { | |||
| }; | |||
| if self.dialect.unnest_as_table_factor() && unnest_input_type.is_some() { | |||
| if let LogicalPlan::Unnest(unnest) = &p.input.as_ref() { | |||
| return self | |||
| .unnest_to_table_factor_sql(unnest, query, select, relation); | |||
| if let LogicalPlan::Projection(projection) = unnest.input.as_ref() | |||
There was a problem hiding this comment.
This feels like something that is a limitation of unnest_to_table_factor_sql -- would it make sense to put the check in there so the check and the code with the assumption are in the same place?
|
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look |
| TestStatementWithDialect { | ||
| sql: "SELECT unnest([1, 2, 3, 4]) from unnest([1, 2, 3]);", | ||
| expected: r#"SELECT UNNEST([1, 2, 3, 4]) AS UNNEST(make_array(Int64(1),Int64(2),Int64(3),Int64(4))) FROM UNNEST([1, 2, 3])"#, | ||
| parser_dialect: Box::new(GenericDialect {}), | ||
| unparser_dialect: Box::new(CustomDialectBuilder::default().with_unnest_as_table_factor(true).build()), | ||
| }, | ||
| TestStatementWithDialect { | ||
| sql: "SELECT unnest([1, 2, 3, 4]) from unnest([1, 2, 3]);", | ||
| expected: r#"SELECT UNNEST([1, 2, 3, 4]) AS UNNEST(make_array(Int64(1),Int64(2),Int64(3),Int64(4))) FROM UNNEST([1, 2, 3])"#, | ||
| parser_dialect: Box::new(GenericDialect {}), | ||
| unparser_dialect: Box::new(CustomDialectBuilder::default().with_unnest_as_table_factor(true).build()), | ||
| }, |
There was a problem hiding this comment.
They look like the same. 🤔 I think we can remove one of them.
* Only unnest source for `EmptyRelation` * Add a note on new condition * Remove new test * Put all unnest assumptions into one function
Which issue does this PR close?
plan_to_sqlare merged #15128Rationale for this change
We currently override source losing some data, see issue for examples
What changes are included in this PR?
Are these changes tested?
Added tests
Are there any user-facing changes?
No