You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "peasee (via GitHub)" <gi...@apache.org> on 2024/10/18 06:53:04 UTC
[PR] fix: Dialect requires derived table alias [datafusion]
peasee opened a new pull request, #12994:
URL: https://github.com/apache/datafusion/pull/12994
* fix: Add Dialect option for requiring table aliases
* feat: Add CustomDialectBuilder for requires_table_alias
* docs: Spelling
* refactor: rename requires_derived_table_alias
* refactor: rename requires_derived_table_alias
## 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 #12993.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Fixes a bug preventing derived logical plans from running in MySQL due to a requirement that every derived table must have an alias:
> The [AS] tbl_name clause is mandatory because every table in a FROM clause must have a name.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Includes a dialect update to specify whether derived tables require aliases. For dialects that do, set a static alias. I've simply selected the name of the operation prefixed with `derived_`, like `derived_sort`, etc.
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Yes
## 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.
-->
No
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "peasee (via GitHub)" <gi...@apache.org>.
peasee commented on code in PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#discussion_r1807298937
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -261,6 +261,24 @@ fn roundtrip_statement_with_dialect() -> Result<()> {
unparser_dialect: Box<dyn UnparserDialect>,
}
let tests: Vec<TestStatementWithDialect> = vec![
+ TestStatementWithDialect {
+ sql: "select min(ta.j1_id) as j1_min from j1 ta order by min(ta.j1_id) limit 10;",
+ expected:
+ // top projection sort gets derived into a subquery
+ // for MySQL, this subquery needs an alias
+ "SELECT `j1_min` FROM (SELECT min(`ta`.`j1_id`) AS `j1_min`, min(`ta`.`j1_id`) FROM `j1` AS `ta` ORDER BY min(`ta`.`j1_id`) ASC) AS `derived_sort` LIMIT 10",
Review Comment:
It is valid for MySQL, I just tested it out in my CLI. I've added both as new cases, thanks!
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "peasee (via GitHub)" <gi...@apache.org>.
peasee commented on PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#issuecomment-2425479967
> > > @goldmedal perhaps you would like to merge this PR as a way to verify your permissions are configured correctly?
> >
> >
> > Sure, I plan to merge this PR if no more comments tomorrow.
>
> Let's hold off on merging this for now. I think I've introduced a regression that I'd like to test more first before merging.
Okay, looks like my PR didn't introduce the regression but there's a somewhat related bug currently on `main`. Take the following SQL:
```
SELECT j1_id FROM (SELECT ta.j1_id AS j1_id FROM j1 ta);
```
This gets rewritten in the `GenericDialect` to:
```
SELECT ta.j1_id FROM (SELECT ta.j1_id FROM j1 AS ta)
```
Which is invalid, because the subquery is un-aliased. I tested it in DuckDB to confirm:
```
D SELECT ta.j1_id FROM (SELECT ta.j1_id FROM j1 ta);
Binder Error: Referenced table "ta" not found!
Candidate tables: "unnamed_subquery"
LINE 1: SELECT ta.j1_id FROM (SELECT ta.j1_id FROM j1 ...
```
Let's merge my PR, and I can raise a new issue for this other bug @goldmedal ?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "peasee (via GitHub)" <gi...@apache.org>.
peasee commented on PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#issuecomment-2425703922
> I see. I think it's an issue but we can fix it in the follow-up PR. Let's merge this PR first. Could you help to file an issue to trace it?
https://github.com/apache/datafusion/issues/13027
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#issuecomment-2423846139
Maybe @phillipleblanc and @sgrebnov want to take a look at this PR.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#issuecomment-2424946473
> @goldmedal perhaps you would like to merge this PR as a way to verify your permissions are configured correctly?
Sure, I plan to merge this PR if no more comments tomorrow.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#issuecomment-2424925770
@goldmedal perhaps you would like to merge this PR as a way to verify your permissions are configured correctly?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#issuecomment-2425708929
Thanks @peasee and @phillipleblanc @alamb for reviewing. 👍
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal merged PR #12994:
URL: https://github.com/apache/datafusion/pull/12994
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#issuecomment-2423720792
By the way, I tried to pull your branch and run the tests on my laptop but the tests always fail. After merging with the latest main branch, the tests are passed.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#issuecomment-2423934854
Looks good to me, thanks @goldmedal and @peasee
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "peasee (via GitHub)" <gi...@apache.org>.
peasee commented on PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#issuecomment-2425681667
> If it's the first one, I think it's an issue because it isn't valid. We can file an issue for it.
My bad, I should've clarified better. It is the first one with `MySqlDialect`. I had just used `GenericDialect` as the example, but it affects both:
```
SELECT `ta`.`j1_id` FROM (SELECT `ta`.`j1_id` FROM `j1` AS `ta`) AS `derived_projection`
```
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#issuecomment-2425690345
> > If it's the first one, I think it's an issue because it isn't valid. We can file an issue for it.
>
> My bad, I should've clarified better. It is the first one with `MySqlDialect`. I had just used `GenericDialect` as the example, but it affects both:
>
> ```
> SELECT `ta`.`j1_id` FROM (SELECT `ta`.`j1_id` FROM `j1` AS `ta`) AS `derived_projection`
> ```
I see. I think it's an issue but we can fix it in the follow-up PR. Let's merge this PR first.
Could you help to file an issue to trace it?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on code in PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#discussion_r1806676727
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -261,6 +261,24 @@ fn roundtrip_statement_with_dialect() -> Result<()> {
unparser_dialect: Box<dyn UnparserDialect>,
}
let tests: Vec<TestStatementWithDialect> = vec![
+ TestStatementWithDialect {
+ sql: "select min(ta.j1_id) as j1_min from j1 ta order by min(ta.j1_id) limit 10;",
+ expected:
+ // top projection sort gets derived into a subquery
+ // for MySQL, this subquery needs an alias
+ "SELECT `j1_min` FROM (SELECT min(`ta`.`j1_id`) AS `j1_min`, min(`ta`.`j1_id`) FROM `j1` AS `ta` ORDER BY min(`ta`.`j1_id`) ASC) AS `derived_sort` LIMIT 10",
Review Comment:
I found the test only covers the `derived_sort` case but some cases aren't covered, such as `derived_projection`, `derived_limit`,...
Could you add more tests for them?
##########
datafusion/sql/src/unparser/plan.rs:
##########
@@ -239,6 +244,22 @@ impl Unparser<'_> {
Ok(())
}
+ fn derive_with_dialect_alias(
+ &self,
+ alias: &str,
+ plan: &LogicalPlan,
+ relation: &mut RelationBuilder,
+ ) -> Result<()> {
+ match self.dialect.requires_derived_table_alias() {
+ false => self.derive(plan, relation, None),
+ true => self.derive(
+ plan,
+ relation,
+ Some(self.new_table_alias(alias.to_string(), vec![])),
+ ),
+ }
Review Comment:
Using `if-else` clause would be clear
```suggestion
if self.dialect.requires_derived_table_alias() {
self.derive(
plan,
relation,
Some(self.new_table_alias(alias.to_string(), vec![])),
)
} else {
self.derive(plan, relation, None)
}
```
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on code in PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#discussion_r1807260135
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -261,6 +261,24 @@ fn roundtrip_statement_with_dialect() -> Result<()> {
unparser_dialect: Box<dyn UnparserDialect>,
}
let tests: Vec<TestStatementWithDialect> = vec![
+ TestStatementWithDialect {
+ sql: "select min(ta.j1_id) as j1_min from j1 ta order by min(ta.j1_id) limit 10;",
+ expected:
+ // top projection sort gets derived into a subquery
+ // for MySQL, this subquery needs an alias
+ "SELECT `j1_min` FROM (SELECT min(`ta`.`j1_id`) AS `j1_min`, min(`ta`.`j1_id`) FROM `j1` AS `ta` ORDER BY min(`ta`.`j1_id`) ASC) AS `derived_sort` LIMIT 10",
Review Comment:
For `derived_projection`, I found the SQL can trigger:
```sql
select j1_id from (select 1 as j1_id)
-> SELECT `j1_id` FROM (SELECT 1 AS `j1_id`) AS `derived_projection`
```
However, I guess it's not a valid SQL for MySQL, right? I tried it in DataFusion and DuckDB, they accept it but Postgres doesn't allow it. Maybe, it could be a `DataFusion` dialect to `MySQL` dialect case.
For `derived_limit`, a similar case can trigger it:
```sql
select * from (select * from j1 limit 10)
-> SELECT * FROM (SELECT * FROM `j1` LIMIT 10) AS `derived_limit`
```
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "peasee (via GitHub)" <gi...@apache.org>.
peasee commented on PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#issuecomment-2425470740
> > @goldmedal perhaps you would like to merge this PR as a way to verify your permissions are configured correctly?
>
> Sure, I plan to merge this PR if no more comments tomorrow.
Let's hold off on merging this for now. I think I've introduced a regression that I'd like to test more first before merging.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#issuecomment-2425671715
>
> This gets rewritten in the `GenericDialect` to:
>
> ```
> SELECT ta.j1_id FROM (SELECT ta.j1_id FROM j1 AS ta)
> ```
>
It's valid for DataFusion
```
> create table j1(j1_id int);
0 row(s) fetched.
Elapsed 0.085 seconds.
> SELECT ta.j1_id FROM (SELECT ta.j1_id FROM j1 AS ta);
+-------+
| j1_id |
+-------+
+-------+
0 row(s) fetched.
Elapsed 0.042 seconds.
```
Generally, `GenericDialect` fits the DataFusion syntax. This behavior makes sense to me.
> Which is invalid, because the subquery is un-aliased. I tested it in DuckDB to confirm:
>
> ```
> D SELECT ta.j1_id FROM (SELECT ta.j1_id FROM j1 ta);
> Binder Error: Referenced table "ta" not found!
> Candidate tables: "unnamed_subquery"
> LINE 1: SELECT ta.j1_id FROM (SELECT ta.j1_id FROM j1 ...
> ```
>
> Let's merge my PR, and I can raise a new issue for this other bug @goldmedal ?
How about `MySQLDialect`? It enables `requires_derived_table_alias`. Which would be the result?
```
SELECT ta.j1_id FROM (SELECT ta.j1_id FROM j1 ta) derived_projection
```
or
```
SELECT j1_id FROM (SELECT ta.j1_id FROM j1 ta) `derived_projection`
```
If it's the first one, I think it's an issue because it isn't valid. We can file an issue for it.
If it's the second one, I think the user can customize the `dialect::requires_derived_table_alias` for the DuckDB syntax purpose.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Dialect requires derived table alias [datafusion]
Posted by "peasee (via GitHub)" <gi...@apache.org>.
peasee commented on code in PR #12994:
URL: https://github.com/apache/datafusion/pull/12994#discussion_r1807069848
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -261,6 +261,24 @@ fn roundtrip_statement_with_dialect() -> Result<()> {
unparser_dialect: Box<dyn UnparserDialect>,
}
let tests: Vec<TestStatementWithDialect> = vec![
+ TestStatementWithDialect {
+ sql: "select min(ta.j1_id) as j1_min from j1 ta order by min(ta.j1_id) limit 10;",
+ expected:
+ // top projection sort gets derived into a subquery
+ // for MySQL, this subquery needs an alias
+ "SELECT `j1_min` FROM (SELECT min(`ta`.`j1_id`) AS `j1_min`, min(`ta`.`j1_id`) FROM `j1` AS `ta` ORDER BY min(`ta`.`j1_id`) ASC) AS `derived_sort` LIMIT 10",
Review Comment:
I added another test case for a more complex SQL that results in 2 nested derives for `derived_sort` and `derived_distinct` in the same query.
I'm not too sure what SQL to use to trigger `derived_limit` or `derived_projection` though... any ideas that I could include?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org