kakiuwang-ui commented on issue #16134:
URL: https://github.com/apache/datafusion/issues/16134#issuecomment-5664333402
Please hold off on closing this — it is only half fixed. The physical-side
check added in #21240 / #22530 covers the default configuration, but the
original behaviour is still reachable.
Reproduced on released 55.1.0:
```
$ datafusion-cli
> SELECT s.*, (SELECT * FROM (VALUES (1), (2)) t(x)) sub FROM (VALUES ('a'),
('b')) s(a);
Execution error: Scalar subquery returned more than one row -- correct
> set datafusion.optimizer.enable_physical_uncorrelated_scalar_subquery =
false;
> SELECT s.*, (SELECT * FROM (VALUES (1), (2)) t(x)) sub FROM (VALUES ('a'),
('b')) s(a);
+---+-----+
| a | sub |
+---+-----+
| a | 1 |
| b | 1 |
| a | 2 |
| b | 2 |
+---+-----+
4 row(s) fetched. -- the bug
this issue reports
```
The code path is unchanged on current main (7b00b63).
**Why:** the cardinality check lives only in `ScalarSubqueryExec`
(`physical-plan/src/scalar_subquery.rs`). `create_initial_plan` deliberately
skips `collect_scalar_subqueries` when the option is off, on the assumption
that `ScalarSubqueryToJoin` rewrote every uncorrelated scalar subquery into a
join, so that operator is never created. `build_join` then rewrites the
subquery as `Left Join: Filter: Boolean(true)`, and its "at most one row"
assumption — stated in the comment there — only actually holds for an aggregate
with no grouping. For a subquery like `SELECT * FROM (VALUES (1), (2))` the
join simply multiplies the outer rows.
**Proposed fix**, unless you would rather it went another way: only
decorrelate an uncorrelated scalar subquery when `subquery.max_rows()` is
`Some(n)` with `n <= 1` — which is exactly the aggregate-without-grouping case
the existing comment relies on — and let anything else fall through to the
physical path so `ScalarSubqueryExec` raises the same error the default
configuration gives. That means `create_initial_plan` should collect scalar
subqueries regardless of the option; the current skip is only an optimisation,
since the collection returns nothing when the rule did rewrite them all.
The alternative would be to reject such subqueries at plan time rather than
at execution time, which would be a different error message from the default
path. I would rather match the default path, but say the word if you prefer
otherwise.
I would like to work on this, along with slt coverage for both settings of
the option. @buraksenn @u70b3 — flagging since you both looked at this.
--
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]
