Unparse `SubqueryAlias` without projections to SQL #12896

goldmedal · 2024-10-12T15:46:21Z

Which issue does this PR close?

Closes #.

Rationale for this change

Consider the following 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

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.

What changes are included in this PR?

When unparsing a SubqueryAlias without any projections, we should create another scope for the alias like

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

Are these changes tested?

yes

Are there any user-facing changes?

The unparsing result is changed.

goldmedal · 2024-10-12T16:00:05Z

After this changed, I found another issue about the anonymous subquery.

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.

findepi · 2024-10-12T19:39:30Z

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"


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?

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.

findepi · 2024-10-15T21:35:18Z

datafusion/sql/tests/cases/plan_to_sql.rs

+    .build()?;
+    let table_scan_with_two_filter = plan_to_sql(&table_scan_with_two_filter)?;
+    assert_eq!(
+        format!("{}", table_scan_with_two_filter),


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)

findepi · 2024-10-15T21:35:52Z

datafusion/sql/tests/cases/plan_to_sql.rs

+    .project(vec![col("id")])?
+    .alias("a")?
+    .build()?;


should this be indented?

It's formatted by the cargo formatter. I think we can't control it 🤔

findepi · 2024-10-15T21:36:09Z

datafusion/sql/tests/cases/plan_to_sql.rs

+        .project(vec![col("id")])?
+        .alias("a")?
+        .build()?;
+    println!("{}", plan.display_indent());


alamb · 2024-10-16T17:29:46Z

datafusion/sql/src/unparser/plan.rs

-                if scan.projection.is_some()
-                    || !scan.filters.is_empty()
-                    || scan.fetch.is_some()
+                if let Some(unparsed_table_scan) =


alamb

Thank you @goldmedal and @findepi for the review

alamb · 2024-10-16T17:30:41Z

FYI @phillipleblanc and @sgrebnov

phillipleblanc

This PR makes sense to me, thanks!

sgrebnov · 2024-10-17T06:48:51Z

LGTM 👍

alamb · 2024-10-17T17:22:09Z

Thanks @sgrebnov @findepi and @phillipleblanc for the review

goldmedal · 2024-10-18T13:40:46Z

Thanks @alamb @sgrebnov @findepi @phillipleblanc !

* change pub function comment to doc * unparse subquery alias without projections * fix tests * rollback the empty line * rollback the empty line * exclude the table_scan with pushdown case * fmt and clippy * simplify the ast to string and remove unused debug code

* init (apache#12453) * Fix unparse table scan with the projection pushdown (apache#12534) * unparse the projection base on the source schema * refactor and enhance the test * Fix unparsing OFFSET (apache#12539) * Unparse Sort with pushdown limit to SQL string (apache#12873) * unparse Sort with push down limit * cargo fmt * set query limit directly * Unparse `SubqueryAlias` without projections to SQL (apache#12896) * change pub function comment to doc * unparse subquery alias without projections * fix tests * rollback the empty line * rollback the empty line * exclude the table_scan with pushdown case * fmt and clippy * simplify the ast to string and remove unused debug code * enhance unparsing plan with pushdown to avoid unnamed subquery (apache#13006) * Update --------- Co-authored-by: Lordworms <[email protected]> Co-authored-by: Jax Liu <[email protected]> Co-authored-by: Justus Flerlage <[email protected]>

goldmedal added 2 commits October 12, 2024 23:11

change pub function comment to doc

813326b

unparse subquery alias without projections

5073d55

goldmedal changed the title ~~Unparse SubqueryAlias without projections to SQL~~ Unparse SubqueryAlias without projections to SQL Oct 12, 2024

github-actions bot added the sql SQL Planner label Oct 12, 2024

goldmedal added 3 commits October 13, 2024 00:03

fix tests

d6c7f70

rollback the empty line

7ec80c2

rollback the empty line

86f2c91

goldmedal marked this pull request as ready for review October 12, 2024 16:42

findepi reviewed Oct 12, 2024

View reviewed changes

goldmedal mentioned this pull request Oct 14, 2024

fix(core): disable optimize_projections rule for SQL generation Canner/wren-engine#822

Merged

goldmedal marked this pull request as draft October 15, 2024 14:57

goldmedal added 2 commits October 16, 2024 01:30

exclude the table_scan with pushdown case

692239c

fmt and clippy

5a178aa

goldmedal marked this pull request as ready for review October 15, 2024 17:52

findepi approved these changes Oct 15, 2024

View reviewed changes

simplify the ast to string and remove unused debug code

8f727cd

alamb reviewed Oct 16, 2024

View reviewed changes

alamb approved these changes Oct 16, 2024

View reviewed changes

phillipleblanc approved these changes Oct 17, 2024

View reviewed changes

alamb merged commit 1ba1e53 into apache:main Oct 17, 2024
24 checks passed

goldmedal deleted the feature/unparse-subalais branch October 18, 2024 13:40

This was referenced Oct 18, 2024

Enhance table scan unparsing to avoid unnamed subqueries. #13006

Merged

fix: Dialect requires derived table alias #12994

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unparse `SubqueryAlias` without projections to SQL #12896

Unparse `SubqueryAlias` without projections to SQL #12896

goldmedal commented Oct 12, 2024

goldmedal commented Oct 12, 2024

findepi Oct 12, 2024

goldmedal Oct 15, 2024

findepi Oct 15, 2024

findepi Oct 15, 2024

goldmedal Oct 16, 2024

findepi Oct 15, 2024

alamb Oct 16, 2024

alamb left a comment

alamb commented Oct 16, 2024

phillipleblanc left a comment

sgrebnov commented Oct 17, 2024

alamb commented Oct 17, 2024

goldmedal commented Oct 18, 2024

Unparse SubqueryAlias without projections to SQL #12896

Unparse SubqueryAlias without projections to SQL #12896

Conversation

goldmedal commented Oct 12, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

goldmedal commented Oct 12, 2024

DataFusion

DuckDB

Postgres

findepi Oct 12, 2024

Choose a reason for hiding this comment

goldmedal Oct 15, 2024

Choose a reason for hiding this comment

findepi Oct 15, 2024

Choose a reason for hiding this comment

findepi Oct 15, 2024

Choose a reason for hiding this comment

goldmedal Oct 16, 2024

Choose a reason for hiding this comment

findepi Oct 15, 2024

Choose a reason for hiding this comment

alamb Oct 16, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb commented Oct 16, 2024

phillipleblanc left a comment

Choose a reason for hiding this comment

sgrebnov commented Oct 17, 2024

alamb commented Oct 17, 2024

goldmedal commented Oct 18, 2024

Unparse `SubqueryAlias` without projections to SQL #12896

Unparse `SubqueryAlias` without projections to SQL #12896