You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "goldmedal (via GitHub)" <gi...@apache.org> on 2024/10/12 15:46:24 UTC
[PR] Unparse SubqueryAlias without projections to SQL [datafusion]
goldmedal opened a new pull request, #12896:
URL: https://github.com/apache/datafusion/pull/12896
## 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 #.
## Rationale for this change
Consider the following SQL
```SQL
SELECT
c.custkey,
c.name
FROM
(
SELECT
c_custkey as custkey,
c_name as name
FROM
customer
) c
```
The unoptimized plan is
```
Projection: c.custkey, c.name
SubqueryAlias: c
Projection: customer.c_custkey AS custkey, customer.c_name AS name
TableScan: customer
```
However, after the `optimize_projections` rule, the top-level projection will be removed like
```
SubqueryAlias: c
Projection: customer.c_custkey AS custkey, customer.c_name AS name
TableScan: customer projection=[c_custkey, c_name]
```
The unparsing result would like
```SQL
SELECT
customer.c_custkey AS custkey,
customer.c_name AS "name"
FROM
(
SELECT
customer.c_custkey,
customer.c_name
FROM
customer
) AS c
```
It's an invalid SQL because the column qualifier is the table name `customer` but it's in the alias `c` scope actually.
<!--
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.
-->
## What changes are included in this PR?
When unparsing a `SubqueryAlias` without any projections, we should create another scope for the alias like
```SQL
SELECT
*
FROM
(
SELECT
customer.c_custkey AS custkey,
customer.c_name AS "name"
FROM
(
SELECT
customer.c_custkey,
customer.c_name
FROM
customer
)
) AS c
```
<!--
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.
-->
## Are these changes tested?
yes
<!--
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)?
-->
## Are there any user-facing changes?
The unparsing result is changed.
<!--
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@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] Unparse `SubqueryAlias` without projections to SQL [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #12896:
URL: https://github.com/apache/datafusion/pull/12896#discussion_r1802046994
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -667,12 +667,95 @@ where
}
#[test]
-fn test_table_scan_pushdown() -> Result<()> {
+fn test_table_scan_alias() -> Result<()> {
let schema = Schema::new(vec![
Field::new("id", DataType::Utf8, false),
Field::new("age", DataType::Utf8, false),
]);
+ let plan = table_scan(Some("t1"), &schema, None)?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
+ let sql = plan_to_sql(&plan)?;
+ assert_eq!(
+ format!("{}", sql),
+ "SELECT * FROM (SELECT t1.id FROM t1) AS a"
+ );
+
+ let plan = table_scan(Some("t1"), &schema, None)?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
+
+ let sql = plan_to_sql(&plan)?;
+ assert_eq!(
+ format!("{}", sql),
+ "SELECT * FROM (SELECT t1.id FROM t1) AS a"
+ );
+
+ let plan = table_scan(Some("t1"), &schema, None)?
+ .filter(col("id").gt(lit(5)))?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
+ println!("{}", plan.display_indent());
Review Comment:
left over?
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -667,12 +667,95 @@ where
}
#[test]
-fn test_table_scan_pushdown() -> Result<()> {
+fn test_table_scan_alias() -> Result<()> {
let schema = Schema::new(vec![
Field::new("id", DataType::Utf8, false),
Field::new("age", DataType::Utf8, false),
]);
+ let plan = table_scan(Some("t1"), &schema, None)?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
+ let sql = plan_to_sql(&plan)?;
+ assert_eq!(
+ format!("{}", sql),
+ "SELECT * FROM (SELECT t1.id FROM t1) AS a"
+ );
+
+ let plan = table_scan(Some("t1"), &schema, None)?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
+
+ let sql = plan_to_sql(&plan)?;
+ assert_eq!(
+ format!("{}", sql),
+ "SELECT * FROM (SELECT t1.id FROM t1) AS a"
+ );
+
+ let plan = table_scan(Some("t1"), &schema, None)?
+ .filter(col("id").gt(lit(5)))?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
+ println!("{}", plan.display_indent());
+ let sql = plan_to_sql(&plan)?;
+ assert_eq!(
+ format!("{}", sql),
+ "SELECT * FROM (SELECT t1.id FROM t1 WHERE (t1.id > 5)) AS a"
+ );
+
+ let table_scan_with_two_filter = table_scan_with_filters(
+ Some("t1"),
+ &schema,
+ None,
+ vec![col("id").gt(lit(1)), col("age").lt(lit(2))],
+ )?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
+ let table_scan_with_two_filter = plan_to_sql(&table_scan_with_two_filter)?;
+ assert_eq!(
+ format!("{}", table_scan_with_two_filter),
Review Comment:
nit: `format!` is unnecessary, just use table_scan_with_two_filter (or table_scan_with_two_filter.to_string()) in assert
(same in other places)
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -667,12 +667,95 @@ where
}
#[test]
-fn test_table_scan_pushdown() -> Result<()> {
+fn test_table_scan_alias() -> Result<()> {
let schema = Schema::new(vec![
Field::new("id", DataType::Utf8, false),
Field::new("age", DataType::Utf8, false),
]);
+ let plan = table_scan(Some("t1"), &schema, None)?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
+ let sql = plan_to_sql(&plan)?;
+ assert_eq!(
+ format!("{}", sql),
+ "SELECT * FROM (SELECT t1.id FROM t1) AS a"
+ );
+
+ let plan = table_scan(Some("t1"), &schema, None)?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
+
+ let sql = plan_to_sql(&plan)?;
+ assert_eq!(
+ format!("{}", sql),
+ "SELECT * FROM (SELECT t1.id FROM t1) AS a"
+ );
+
+ let plan = table_scan(Some("t1"), &schema, None)?
+ .filter(col("id").gt(lit(5)))?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
+ println!("{}", plan.display_indent());
+ let sql = plan_to_sql(&plan)?;
+ assert_eq!(
+ format!("{}", sql),
+ "SELECT * FROM (SELECT t1.id FROM t1 WHERE (t1.id > 5)) AS a"
+ );
+
+ let table_scan_with_two_filter = table_scan_with_filters(
+ Some("t1"),
+ &schema,
+ None,
+ vec![col("id").gt(lit(1)), col("age").lt(lit(2))],
+ )?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
Review Comment:
should this be indented?
--
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] Unparse `SubqueryAlias` without projections to SQL [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12896:
URL: https://github.com/apache/datafusion/pull/12896#discussion_r1803532406
##########
datafusion/sql/src/unparser/plan.rs:
##########
@@ -249,12 +248,9 @@ impl Unparser<'_> {
) -> Result<()> {
match plan {
LogicalPlan::TableScan(scan) => {
- if scan.projection.is_some()
- || !scan.filters.is_empty()
- || scan.fetch.is_some()
+ if let Some(unparsed_table_scan) =
Review Comment:
Nice
--
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] Unparse `SubqueryAlias` without projections to SQL [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #12896:
URL: https://github.com/apache/datafusion/pull/12896#discussion_r1797766180
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -697,7 +739,7 @@ fn test_table_scan_pushdown() -> Result<()> {
plan_to_sql(&table_scan_with_projection_alias)?;
assert_eq!(
format!("{}", table_scan_with_projection_alias),
- "SELECT ta.id, ta.age FROM t1 AS ta"
+ "SELECT * FROM (SELECT ta.id, ta.age FROM t1 AS ta) AS ta"
Review Comment:
That's not an improvement, right?
I am OK if this is necessary, but is there a different way to fix the original problem, without affecting here?
--
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] Unparse `SubqueryAlias` without projections to SQL [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #12896:
URL: https://github.com/apache/datafusion/pull/12896#issuecomment-2422505174
Thanks @alamb @sgrebnov @findepi @phillipleblanc !
--
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] Unparse `SubqueryAlias` without projections to SQL [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #12896:
URL: https://github.com/apache/datafusion/pull/12896
--
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] Unparse `SubqueryAlias` without projections to SQL [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #12896:
URL: https://github.com/apache/datafusion/pull/12896#issuecomment-2408609989
After this changed, I found another issue about the anonymous subquery.
```sql
SELECT
*
FROM
(
SELECT
customer.c_custkey AS custkey,
customer.c_name AS "name"
FROM
(
SELECT
customer.c_custkey,
customer.c_name
FROM
customer
)
) AS c
```
This SQL is valid for DataFusion but it isn't valid for Postgres or DuckDB. The deepest-level subquery is anonymous, but its outer projection uses its column with the qualifier `customer.c_custkey AS custkey, `. For DuckDB and Postgres, this kind of anonymous subquery (a.k.a unnamed subquery) is invalid.
## DataFusion
```
DataFusion CLI v42.0.0
> create table customer(c_custkey int, c_name varchar);
0 row(s) fetched.
Elapsed 0.009 seconds.
> SELECT
*
FROM
(
SELECT
customer.c_custkey AS custkey,
customer.c_name AS "name"
FROM
(
SELECT
customer.c_custkey,
customer.c_name
FROM
customer
)
) AS c;
+---------+------+
| custkey | name |
+---------+------+
+---------+------+
0 row(s) fetched.
Elapsed 0.012 seconds.
```
## DuckDB
```
D create table customer(c_custkey int, c_name varchar);
D SELECT
*
FROM
(
SELECT
customer.c_custkey AS custkey,
customer.c_name AS "name"
FROM
(
SELECT
customer.c_custkey,
customer.c_name
FROM
customer
)
) AS c;
Binder Error: Referenced table "customer" not found!
Candidate tables: "unnamed_subquery"
LINE 6: customer.c_custkey AS custkey,
```
## Postgres
```
test=# create table customer(c_custkey int, c_name varchar);
CREATE TABLE
test=# SELECT
*
FROM
(
SELECT
customer.c_custkey AS custkey,
customer.c_name AS "name"
FROM
(
SELECT
customer.c_custkey,
customer.c_name
FROM
customer
)
) AS c;
ERROR: subquery in FROM must have an alias
LINE 9: (
^
HINT: For example, FROM (SELECT ...) [AS] foo.
```
For some SQL pushdown purposes, we are better off generating SQL with the common syntax. I'll file an issue and try to fix 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] Unparse `SubqueryAlias` without projections to SQL [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on code in PR #12896:
URL: https://github.com/apache/datafusion/pull/12896#discussion_r1802242261
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -667,12 +667,95 @@ where
}
#[test]
-fn test_table_scan_pushdown() -> Result<()> {
+fn test_table_scan_alias() -> Result<()> {
let schema = Schema::new(vec![
Field::new("id", DataType::Utf8, false),
Field::new("age", DataType::Utf8, false),
]);
+ let plan = table_scan(Some("t1"), &schema, None)?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
+ let sql = plan_to_sql(&plan)?;
+ assert_eq!(
+ format!("{}", sql),
+ "SELECT * FROM (SELECT t1.id FROM t1) AS a"
+ );
+
+ let plan = table_scan(Some("t1"), &schema, None)?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
+
+ let sql = plan_to_sql(&plan)?;
+ assert_eq!(
+ format!("{}", sql),
+ "SELECT * FROM (SELECT t1.id FROM t1) AS a"
+ );
+
+ let plan = table_scan(Some("t1"), &schema, None)?
+ .filter(col("id").gt(lit(5)))?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
+ println!("{}", plan.display_indent());
+ let sql = plan_to_sql(&plan)?;
+ assert_eq!(
+ format!("{}", sql),
+ "SELECT * FROM (SELECT t1.id FROM t1 WHERE (t1.id > 5)) AS a"
+ );
+
+ let table_scan_with_two_filter = table_scan_with_filters(
+ Some("t1"),
+ &schema,
+ None,
+ vec![col("id").gt(lit(1)), col("age").lt(lit(2))],
+ )?
+ .project(vec![col("id")])?
+ .alias("a")?
+ .build()?;
Review Comment:
It's formatted by the cargo formatter. I think we can't control 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] Unparse `SubqueryAlias` without projections to SQL [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12896:
URL: https://github.com/apache/datafusion/pull/12896#issuecomment-2420095311
Thanks @sgrebnov @findepi and @phillipleblanc for the review
--
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] Unparse `SubqueryAlias` without projections to SQL [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on code in PR #12896:
URL: https://github.com/apache/datafusion/pull/12896#discussion_r1801666802
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -697,7 +739,7 @@ fn test_table_scan_pushdown() -> Result<()> {
plan_to_sql(&table_scan_with_projection_alias)?;
assert_eq!(
format!("{}", table_scan_with_projection_alias),
- "SELECT ta.id, ta.age FROM t1 AS ta"
+ "SELECT * FROM (SELECT ta.id, ta.age FROM t1 AS ta) AS ta"
Review Comment:
Thanks @findepi. Sorry for replying so late 🙇
You're right. I shouldn't break the result of the table scan with any pushdown. I stopped projecting a wildcard if the `SubqueryAlias`'s child is a pushdown table scan.
--
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] Unparse `SubqueryAlias` without projections to SQL [datafusion]
Posted by "sgrebnov (via GitHub)" <gi...@apache.org>.
sgrebnov commented on PR #12896:
URL: https://github.com/apache/datafusion/pull/12896#issuecomment-2418668449
LGTM 👍
--
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] Unparse `SubqueryAlias` without projections to SQL [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12896:
URL: https://github.com/apache/datafusion/pull/12896#issuecomment-2417472507
FYI @phillipleblanc and @sgrebnov
--
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