-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Reuse alias if possible #14781
base: main
Are you sure you want to change the base?
Reuse alias if possible #14781
Conversation
Interestingly, I was working on a very similar PR last night: |
yes, @alamb, I think we got on the same issue with |
Yes, indeed -- something is going on with unnest. It would be great if you wanted to take over Feel free to pull over the test case from https://github.com/apache/datafusion/pull/15008/files as well |
datafusion/sql/src/unparser/plan.rs
Outdated
|
||
unnest_relation.alias(Some(self.new_table_alias(alias.to_string(), vec![]))); | ||
|
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.
@goldmedal FYI, this is another change to unnest - I want to always do alias to make behaviour more consistent
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 don't get why this way can make behavior more consistent 🤔. Could you explain more?
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.
BTW, I'm considering how to fix the issue mentioned in (#15090 (comment)). I think add an alias for the unnest may be a good idea.
I think it should like
unnest_relation.alias(Some(self.new_table_alias(alias.to_string(), vec![]))); | |
unnest_relation.alias(Some(self.new_table_alias("unnset_table_1", vec![alias.to_string()]))); |
Then, we can get the result like
SELECT "UNNEST(make_array(Int64(1),Int64(2),Int64(3)))" FROM UNNEST([1, 2, 3]) AS unnest_table ("UNNEST(make_array(Int64(1),Int64(2),Int64(3)))")
It can not only fit what you want to do but generate a valid SQL. WDYT?
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.
#15233
I created an issue to explain more.
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 is an edge case example
SELECT * from UNNEST([1,2,3]), UNNEST([1,2,3,4])
is transformed into
SELECT UNNEST(make_array(Int64(1),Int64(2),Int64(3))), UNNEST(make_array(Int64(1),Int64(2),Int64(3),Int64(4))) FROM UNNEST([1, 2, 3]) AS unnset_table_1 (UNNEST(make_array(Int64(1),Int64(2),Int64(3)))) CROSS JOIN UNNEST([1, 2, 3, 4]) AS unnset_table_1 (UNNEST(make_array(Int64(1),Int64(2),Int64(3),Int64(4))))
# Conflicts: # datafusion/expr/src/expr.rs # datafusion/sql/src/unparser/plan.rs # datafusion/sql/tests/cases/plan_to_sql.rs
Which issue does this PR close?
Rationale for this change
Currently, alias over alias creates an extra expression layer, which gets merged in
optimize_projections
through an expensive recursive functionWhat changes are included in this PR?
A small change to reuse an existing alias when possible. This affects two cases:
optimize_projections
isn't called (e.g., when there's only one projection andmerge_consecutive_projections
isn't run)Are these changes tested?
Extended doctest
Are there any user-facing changes?
No.