-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix ambiguous reference when aliasing in combination with ORDER BY
#8425
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
Changes from all commits
7afeb8b
6332bec
cc5e0c7
a114310
928c811
839093e
a836cde
5648dc7
a670409
22894a3
73a59d2
46409c2
8a86a4c
cf5c584
62ae9b9
da02fa2
d98eb2e
79e7216
ba51abd
2468f52
180c303
68980ba
9411940
ba28346
df0942f
edccb66
fb74b99
767b004
2e0eef5
749e0c8
9e65482
443595c
5d43a94
b6d85d7
e6f144e
dd5af6d
d755ce6
a338ac2
47b81e6
2f39667
0f65aca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3546,13 +3546,24 @@ fn test_select_unsupported_syntax_errors(#[case] sql: &str, #[case] error: &str) | |
| fn select_order_by_with_cast() { | ||
| let sql = | ||
| "SELECT first_name AS first_name FROM (SELECT first_name AS first_name FROM person) ORDER BY CAST(first_name as INT)"; | ||
| let expected = "Sort: CAST(first_name AS first_name AS Int32) ASC NULLS LAST\ | ||
| \n Projection: first_name AS first_name\ | ||
| \n Projection: person.first_name AS first_name\ | ||
| let expected = "Sort: CAST(person.first_name AS Int32) ASC NULLS LAST\ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this certainly looks better |
||
| \n Projection: person.first_name\ | ||
| \n Projection: person.first_name\ | ||
| \n TableScan: person"; | ||
| quick_test(sql, expected); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_avoid_add_alias() { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure logical consistency |
||
| // avoiding adding an alias if the column name is the same. | ||
| // plan1 = plan2 | ||
| let sql = "select person.id as id from person order by person.id"; | ||
| let plan1 = logical_plan(sql).unwrap(); | ||
| let sql = "select id from person order by id"; | ||
| let plan2 = logical_plan(sql).unwrap(); | ||
| assert_eq!(format!("{plan1:?}"), format!("{plan2:?}")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_duplicated_left_join_key_inner_join() { | ||
| // person.id * 2 happen twice in left side. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -868,6 +868,21 @@ statement error DataFusion error: Error during planning: EXCLUDE or EXCEPT conta | |
| SELECT * EXCLUDE(d, b, c, a, a, b, c, d) | ||
| FROM table1 | ||
|
|
||
| # avoiding adding an alias if the column name is the same | ||
| query TT | ||
| EXPLAIN select a as a FROM table1 order by a | ||
| ---- | ||
| logical_plan | ||
| Sort: table1.a ASC NULLS LAST | ||
| --TableScan: table1 projection=[a] | ||
| physical_plan | ||
| SortExec: expr=[a@0 ASC NULLS LAST] | ||
| --MemoryExec: partitions=1, partition_sizes=[1] | ||
|
|
||
| # ambiguous column references in on join | ||
| query error DataFusion error: Schema error: Ambiguous reference to unqualified field a | ||
| EXPLAIN select a as a FROM table1 t1 CROSS JOIN table1 t2 order by a | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added |
||
|
|
||
| # run below query in multi partitions | ||
| statement ok | ||
| set datafusion.execution.target_partitions = 2; | ||
|
|
||
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.
Can you please add a comment here about what this is doing? I think it is avoiding adding an alias if the column name is the same.
Do we have to worry about relation names here too (like what if
column.relationis non null?)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 understand, but I didn't expect the scenario where
column.relationis inconsistent🤔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 was thinking of a query like
Where both table1 and table2 have a column named
a. I think we should add a test for this case too -- I'll try and do so later todayI would expect the test to fail
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.
as you expected