dreverri commented on code in PR #27333:
URL: https://github.com/apache/datafusion/pull/27333#discussion_r4552721614
##########
datafusion/sql/src/unparser/plan.rs:
##########
Review Comment:
The `optimize_projections` step should only produce an empty `Projection`
when it can prove that the parent node needs only the row count of its input,
not any column values. In the PR's example, `select count(*) from t where id >
10`, the aggregate `count(Int64(1))` reads no columns. The filter below it
still needs `id`, so the optimizer keeps `id` for the filter and inserts an
empty `Projection` above it:
```
Projection: count(Int64(1)) AS count(*)
Aggregate: groupBy=[[]], aggr=[[count(Int64(1))]]
Projection: <-- empty
Filter: t.id > Int32(10)
TableScan: t projection=[id]
```
Because the parent was planned against a zero-column input, nothing above
the empty `Projection` can reference a column, including the `1` the unparser
adds. `SELECT 1 FROM ...` returns the same rows as `SELECT FROM ...`, so the
result is unchanged.
To check the cases you mentioned, I ran zero-column queries in DataFusion
and PostgreSQL 17, and ran the default-dialect unparser output for the same
plans, which is the same query with `SELECT FROM` replaced by `SELECT 1`, back
through DataFusion. `t` has `id` values 1, 5, 11, 12, 20.
| Query (zero-column form) | DataFusion | PostgreSQL | DataFusion, unparsed
(`SELECT 1`) |
|---|---|---|---|
| `SELECT t.id FROM t WHERE EXISTS (SELECT FROM t WHERE t.id > 10) ORDER BY
t.id` | 5 rows | 5 rows | 5 rows |
| `SELECT t.id FROM t WHERE EXISTS (SELECT FROM t WHERE t.id > 100) ORDER BY
t.id` | 0 rows | 0 rows | 0 rows |
| `SELECT t.id FROM t WHERE NOT EXISTS (SELECT FROM t WHERE t.id > 10) ORDER
BY t.id` | 0 rows | 0 rows | 0 rows |
| `SELECT count(1) AS n FROM (SELECT FROM t WHERE t.id > 10 UNION ALL SELECT
FROM t WHERE t.id < 6) s` | n = 5 | n = 5 | n = 5 |
| `SELECT count(1) AS n FROM (SELECT FROM t WHERE t.id > 10 UNION SELECT
FROM t WHERE t.id < 6) s` | n = 1 | n = 1 | n = 1 |
| `SELECT count(1) AS n FROM (SELECT FROM t WHERE t.id > 100 UNION SELECT
FROM t WHERE t.id > 100) s` | n = 0 | n = 0 | n = 0 |
The one visible difference is when the empty projection is the **root** of
the plan, e.g. a user explicitly asks for zero columns. Then the SQL returns
one constant column instead of zero. That's the same trade-off `TableScan` has
had since #19221, and it only applies to dialects with
`supports_empty_select_list() == false`, where `SELECT FROM` isn't valid
anyway. PostgreSQL still gets `SELECT FROM`.
This PR also fixes a union case. Before it, an empty `TableScan` input and
an empty `Projection` input unparsed as `SELECT 1 FROM t UNION ALL SELECT FROM
t WHERE ...`, which fails with `UNION queries have different number of
columns`. Both inputs now use the same fallback.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
