adriangb commented on issue #4610:
URL: https://github.com/apache/datafusion/issues/4610#issuecomment-5611065807
This still reproduces on 55.0.0 and on current `main` (`40488988`, today).
Adding a reproducer, the current error text, and the mechanism, since this
issue has none of them.
There is also a second problem alongside the missing check, which the title
does not cover: the error the user actually gets.
A window function used as a `HAVING` predicate is accepted by the SQL
planner and by logical planning, and then fails during physical planning.
Two separate problems:
1. **The query should be rejected at planning time.** `HAVING` is evaluated
before window functions are computed, so a window function cannot appear there.
PostgreSQL rejects it outright with `window functions are not allowed in
HAVING`. DataFusion instead builds a `Filter` whose predicate contains an
`Expr::WindowFunction`, which has no physical equivalent, and fails late.
2. **The error text is unusable.** It is the `Debug` formatting of the
internal `Expr::WindowFunction`, just under 2,000 characters on one line,
covering the whole `Signature` coercion table, `WindowFunctionParams` and
`WindowFrame`. It never mentions `HAVING`, the offending function, or the
offending column, so nothing in it points the user back at their SQL.
This is narrowly about rejecting an illegal placement, not about supporting
anything new. The same expression is legal and already works in the `SELECT`
list, and DataFusion already supports `QUALIFY`, which is the correct clause
for exactly this predicate and plans and runs correctly. `WHERE` behaves the
same way as `HAVING`.
### Reproducer
One `datafusion-cli` session, no data files required:
```console
$ datafusion-cli
DataFusion CLI v55.0.0
> CREATE TABLE t(a INT, b INT) AS VALUES (1, 1), (2, 2);
0 row(s) fetched.
> SELECT a, count(*) AS cnt, sum(count(*)) OVER () AS total
FROM t
GROUP BY a
HAVING sum(count(*)) OVER () >= 10;
This feature is not implemented: Physical plan does not support logical
expression WindowFunction(WindowFunction { fun: AggregateUDF(AggregateUDF {
inner: Sum { signature: Signature { type_signature: OneOf([Coercible([Exact {
desired_type: Decimal, encoding_preservation: EncodingPreservation {
preserve_dictionary: false } }]), Coercible([Implicit { desired_type:
Native(LogicalType(Native(UInt64), UInt64)), implicit_coercion:
ImplicitCoercion { allowed_source_types: [Native(LogicalType(Native(UInt8),
UInt8)), Native(LogicalType(Native(UInt16), UInt16)),
Native(LogicalType(Native(UInt32), UInt32))], default_casted_type: UInt64 },
encoding_preservation: EncodingPreservation { preserve_dictionary: false } }]),
Coercible([Implicit { desired_type: Native(LogicalType(Native(Int64), Int64)),
implicit_coercion: ImplicitCoercion { allowed_source_types:
[Native(LogicalType(Native(Int8), Int8)), Native(LogicalType(Native(Int16),
Int16)), Native(LogicalType(Native(Int32), Int32))], default_cas
ted_type: Int64 }, encoding_preservation: EncodingPreservation {
preserve_dictionary: false } }]), Coercible([Implicit { desired_type:
Native(LogicalType(Native(Float64), Float64)), implicit_coercion:
ImplicitCoercion { allowed_source_types: [Float], default_casted_type: Float64
}, encoding_preservation: EncodingPreservation { preserve_dictionary: false }
}]), Coercible([Exact { desired_type: Duration, encoding_preservation:
EncodingPreservation { preserve_dictionary: false } }]), Coercible([Exact {
desired_type: Interval, encoding_preservation: EncodingPreservation {
preserve_dictionary: false } }])]), volatility: Immutable, parameter_names:
None } } }), params: WindowFunctionParams { args: [Column(Column { relation:
None, name: "count(Int64(1))" })], partition_by: [], order_by: [],
window_frame: WindowFrame { units: Rows, start_bound: Preceding(UInt64(NULL)),
end_bound: Following(UInt64(NULL)), is_causal: false }, filter: None,
null_treatment: None, distinct: false } })
```
That is a single 1,981-character line. The shortest form of the same
failure, with a nullary window function, is still 526 characters and equally
uninformative:
```console
> SELECT a FROM t GROUP BY a HAVING row_number() OVER () = 1;
This feature is not implemented: Physical plan does not support logical
expression WindowFunction(WindowFunction { fun: WindowUDF(WindowUDF { inner:
RowNumber { signature: Signature { type_signature: Nullary, volatility:
Immutable, parameter_names: None } } }), params: WindowFunctionParams { args:
[], partition_by: [], order_by: [], window_frame: WindowFrame { units: Rows,
start_bound: Preceding(UInt64(NULL)), end_bound: Following(UInt64(NULL)),
is_causal: false }, filter: None, null_treatment: None, distinct: false } })
```
Variants, all one-liners against the same table:
| Query | Result |
|---|---|
| `SELECT a, count(*) AS cnt, sum(count(*)) OVER () AS total FROM t GROUP BY
a HAVING sum(count(*)) OVER () >= 10;` | fails at physical planning |
| `SELECT a FROM t GROUP BY a HAVING row_number() OVER () = 1;` | fails at
physical planning |
| `SELECT a FROM t WHERE row_number() OVER () = 1;` | fails at physical
planning |
| `SELECT a, count(*) AS cnt, sum(count(*)) OVER () AS total FROM t GROUP BY
a;` | works (correct: window over aggregate in the projection) |
| `SELECT a, count(*) AS cnt, sum(count(*)) OVER () AS total FROM t GROUP BY
a QUALIFY sum(count(*)) OVER () >= 2;` | works (correct: `QUALIFY` is the
clause for this) |
| `SELECT * FROM (SELECT a, count(*) AS cnt, sum(count(*)) OVER () AS total
FROM t GROUP BY a) s WHERE total >= 2;` | works (correct: filter in an outer
query) |
`EXPLAIN` cannot show the accepted plan: it fails the same way even with
`datafusion.explain.logical_plan_only = true`.
### Expected behavior
The query should fail while the logical plan is built, with a message that
names the problem. PostgreSQL 17.11, for comparison:
```
postgres=# SELECT a, count(*) AS cnt, sum(count(*)) OVER () AS total FROM t
GROUP BY a HAVING sum(count(*)) OVER () >= 10;
ERROR: window functions are not allowed in HAVING
LINE 1: ...unt(*)) OVER () AS total FROM t GROUP BY a HAVING sum(count(...
^
postgres=# SELECT a FROM t WHERE row_number() OVER () = 1;
ERROR: window functions are not allowed in WHERE
LINE 1: SELECT a FROM t WHERE row_number() OVER () = 1;
^
```
DataFusion already produces exactly this kind of message for the
neighbouring illegal-nesting case, so the target is well established in the
codebase:
```console
> SELECT sum(sum(1));
Error during planning: Aggregate function calls cannot be nested:
'sum(Int64(1))' is nested inside 'sum(sum(Int64(1)))'
```
Something along the lines of `Error during planning: Window function calls
are not allowed in HAVING: 'sum(count(*)) ROWS BETWEEN UNBOUNDED PRECEDING AND
UNBOUNDED FOLLOWING'` would be equally actionable here. Since DataFusion
already supports `QUALIFY`, pointing at it in a `Diagnostic` help message would
make the fix self-explanatory.
The legal cases in the table above must keep working, in particular
`QUALIFY` and a window function over an aggregate in the projection.
### Mechanism and prior art
Versions tested: `datafusion-cli` 55.0.0 (latest release) and `main` at
`40488988`, which fail identically; also 54.0.0, which fails the same way.
PostgreSQL 17.11 for the comparison above.
Mechanism, as far as I traced it:
- The `HAVING` predicate becomes a `Filter` above the `Aggregate`
(`select_to_plan`, `datafusion/sql/src/select.rs`).
- `Filter::try_new_internal` (`datafusion/expr/src/logical_plan/plan.rs`)
validates only that the predicate's type is boolean, so a predicate containing
`Expr::WindowFunction` is accepted.
- `DefaultPhysicalPlanner` then calls `create_physical_expr` on that
predicate. `Expr::WindowFunction` has no physical-expression equivalent, so it
falls through to the catch-all `not_impl_err!("Physical plan does not support
logical expression {other:?}")` in `datafusion/physical-expr/src/planner.rs`,
which is where the `Debug` dump comes from.
There is prior art in the codebase for the shape a fix could take. #23813
added a crate-private `check_aggregate_and_window_nesting` in
`datafusion/expr/src/utils.rs`, called from `Aggregate::try_new` and
`Window::try_new`, which returns a `plan_err!` naming both expressions and
carries a `Diagnostic` with a span into the original SQL. That check is about
one call nested inside another, so it does not cover a window call appearing in
a `Filter` predicate. An equivalent placement check in `Filter::try_new` would
cover both the SQL path and the `DataFrame`/`LogicalPlanBuilder` path.
--
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]
