-
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
Support unparsing UNNEST
plan to UNNEST
table factor SQL
#13660
Support unparsing UNNEST
plan to UNNEST
table factor SQL
#13660
Conversation
sql: "SELECT UNNEST([1,2,3])", | ||
expected: r#"SELECT * FROM UNNEST([1, 2, 3])"#, |
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.
Because the logical plan of SELECT UNNEST([1,2,3])
and SELECT * FROM UNNEST([1, 2, 3])
is the same, if the with_unnest_as_table_factor
is enabled, we can't avoid to unparse it to the table factor SQL. However, I guess it's fine because they are the same in most scenarios. 🤔
/// Allow to unparse the unnest plan as [ast::TableFactor::UNNEST]. | ||
/// | ||
/// Some dialects like BigQuery require UNNEST to be used in the FROM clause but | ||
/// the LogicalPlan planner always put UNNEST in the SELECT clause. This flag allows |
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.
put -> puts
well, UNNEST is a lateral relational operator. It doesn't belong to the select clause.
It's probably yet another witness of the problem that the LP is half way between AST and a good IR (which we don't have yet)
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.
well, UNNEST is a lateral relational operator. It doesn't belong to the select clause. It's probably yet another witness of the problem that the LP is half way between AST and a good IR (which we don't have yet)
Interesting.
The logical planner plans FROM unnest([1,2,3])
to
Projection: unnest_placeholder(make_array(Int64(1),Int64(2),Int64(3)),depth=1) AS UNNEST(make_array(Int64(1),Int64(2),Int64(3)))
Unnest: lists[unnest_placeholder(make_array(Int64(1),Int64(2),Int64(3)))|depth=1] structs[]
Projection: make_array(Int64(1), Int64(2), Int64(3)) AS unnest_placeholder(make_array(Int64(1),Int64(2),Int64(3)))
EmptyRelation
Do you think it's a good plan? What would a good IR look like? 🤔
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.
It's not unreasonable.
Looking at this the "unnest_placeholder" looks more like "unnest input".
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.
actually it's both: unnest input and unnest output. confusing.
Thanks @findepi for reviewing 👍 |
🚀 |
…13660) * add `unnest_as_table_factor` and `UnnestRelationBuilder` * unparse unnest as table factor * fix typo * add tests for the default configs * add a static const for unnest_placeholder * fix tests * fix tests
…13660) * add `unnest_as_table_factor` and `UnnestRelationBuilder` * unparse unnest as table factor * fix typo * add tests for the default configs * add a static const for unnest_placeholder * fix tests * fix tests
Which issue does this PR close?
Closes #13601
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?