You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Asura7969 (via GitHub)" <gi...@apache.org> on 2023/12/05 11:18:38 UTC
[PR] Fix ambiguous reference when aliasing in combination with `ORDER BY` [arrow-datafusion]
Asura7969 opened a new pull request, #8425:
URL: https://github.com/apache/arrow-datafusion/pull/8425
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #8391.
## Rationale for this change
```sql
❯ CREATE TABLE t (a INTEGER);
0 rows in set. Query took 0.003 seconds.
❯ SELECT t.a AS a FROM t ORDER BY t.a;
Schema error: Schema contains qualified field name t.a and unqualified field name a which would be ambiguous
```
## What changes are included in this PR?
When the field alias is consistent with the original name, cancel the alias
## Are these changes tested?
Overwrite existing tests
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ambiguous reference when aliasing in combination with `ORDER BY` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8425:
URL: https://github.com/apache/arrow-datafusion/pull/8425
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ambiguous reference when aliasing in combination with `ORDER BY` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8425:
URL: https://github.com/apache/arrow-datafusion/pull/8425#discussion_r1416248303
##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -3546,9 +3546,9 @@ 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\
Review Comment:
this certainly looks better
##########
datafusion/sql/src/select.rs:
##########
@@ -384,7 +384,11 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
&[&[plan.schema()]],
&plan.using_columns()?,
)?;
- let expr = col.alias(self.normalizer.normalize(alias));
+ let name = self.normalizer.normalize(alias);
+ let expr = match &col {
Review Comment:
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.relation` is non null?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ambiguous reference when aliasing in combination with `ORDER BY` [arrow-datafusion]
Posted by "Asura7969 (via GitHub)" <gi...@apache.org>.
Asura7969 commented on code in PR #8425:
URL: https://github.com/apache/arrow-datafusion/pull/8425#discussion_r1416605213
##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -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\
+ \n Projection: person.first_name\
+ \n Projection: person.first_name\
\n TableScan: person";
quick_test(sql, expected);
}
+#[test]
+fn test_avoid_add_alias() {
Review Comment:
Ensure logical consistency
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ambiguous reference when aliasing in combination with `ORDER BY` [arrow-datafusion]
Posted by "Asura7969 (via GitHub)" <gi...@apache.org>.
Asura7969 commented on code in PR #8425:
URL: https://github.com/apache/arrow-datafusion/pull/8425#discussion_r1417481493
##########
datafusion/sqllogictest/test_files/select.slt:
##########
@@ -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
Review Comment:
I added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ambiguous reference when aliasing in combination with `ORDER BY` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8425:
URL: https://github.com/apache/arrow-datafusion/pull/8425#discussion_r1417265589
##########
datafusion/sql/src/select.rs:
##########
@@ -384,7 +384,11 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
&[&[plan.schema()]],
&plan.using_columns()?,
)?;
- let expr = col.alias(self.normalizer.normalize(alias));
+ let name = self.normalizer.normalize(alias);
+ let expr = match &col {
Review Comment:
I was thinking of a query like
```
EXPLAIN select a as a FROM table1 CROSS JOIN table2 order by a
```
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 today
I would expect the test to fail
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ambiguous reference when aliasing in combination with `ORDER BY` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8425:
URL: https://github.com/apache/arrow-datafusion/pull/8425#issuecomment-1841580587
fyi @Jesse-Bakker
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ambiguous reference when aliasing in combination with `ORDER BY` [arrow-datafusion]
Posted by "Asura7969 (via GitHub)" <gi...@apache.org>.
Asura7969 commented on code in PR #8425:
URL: https://github.com/apache/arrow-datafusion/pull/8425#discussion_r1416584642
##########
datafusion/sql/src/select.rs:
##########
@@ -384,7 +384,11 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
&[&[plan.schema()]],
&plan.using_columns()?,
)?;
- let expr = col.alias(self.normalizer.normalize(alias));
+ let name = self.normalizer.normalize(alias);
+ let expr = match &col {
Review Comment:
I understand, but I didn't expect the scenario where `column.relation` is inconsistent🤔
##########
datafusion/sql/src/select.rs:
##########
@@ -384,7 +384,11 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
&[&[plan.schema()]],
&plan.using_columns()?,
)?;
- let expr = col.alias(self.normalizer.normalize(alias));
+ let name = self.normalizer.normalize(alias);
+ let expr = match &col {
Review Comment:
I understand, but I didn't expect the scenario where `column.relation` is inconsistent🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ambiguous reference when aliasing in combination with `ORDER BY` [arrow-datafusion]
Posted by "Asura7969 (via GitHub)" <gi...@apache.org>.
Asura7969 commented on code in PR #8425:
URL: https://github.com/apache/arrow-datafusion/pull/8425#discussion_r1417464447
##########
datafusion/sql/src/select.rs:
##########
@@ -384,7 +384,11 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
&[&[plan.schema()]],
&plan.using_columns()?,
)?;
- let expr = col.alias(self.normalizer.normalize(alias));
+ let name = self.normalizer.normalize(alias);
+ let expr = match &col {
Review Comment:
as you expected
```sql
DataFusion CLI v33.0.0
❯ CREATE TABLE table1 (a int) as values(1);
0 rows in set. Query took 0.037 seconds.
❯ CREATE TABLE table2 (a int) as values(1);
0 rows in set. Query took 0.012 seconds.
❯ EXPLAIN select a as a FROM table1 CROSS JOIN table2 order by a;
Schema error: Ambiguous reference to unqualified field a
```
##########
datafusion/sql/src/select.rs:
##########
@@ -384,7 +384,11 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
&[&[plan.schema()]],
&plan.using_columns()?,
)?;
- let expr = col.alias(self.normalizer.normalize(alias));
+ let name = self.normalizer.normalize(alias);
+ let expr = match &col {
Review Comment:
as you expected
```sql
DataFusion CLI v33.0.0
❯ CREATE TABLE table1 (a int) as values(1);
0 rows in set. Query took 0.037 seconds.
❯ CREATE TABLE table2 (a int) as values(1);
0 rows in set. Query took 0.012 seconds.
❯ EXPLAIN select a as a FROM table1 CROSS JOIN table2 order by a;
Schema error: Ambiguous reference to unqualified field a
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org