fix: Coerce aggregate FILTER predicates to boolean#22774
Conversation
nuno-faria
left a comment
There was a problem hiding this comment.
Thanks @pchintar, LGTM.
Just a small note: predicates like select count(*) filter (where 1) are also converted to bool. DuckDB follows the same logic, while Postgres returns an error. I'd say this is ok.
| # window_aggregate_with_untyped_null_filter | ||
| query I | ||
| SELECT count(*) FILTER (WHERE NULL) OVER () | ||
| FROM (VALUES (1)) AS t(x) | ||
| ---- | ||
| 0 | ||
|
|
||
| query I | ||
| SELECT sum(1) FILTER (WHERE NULL) OVER () | ||
| FROM (VALUES (1)) AS t(x) | ||
| ---- | ||
| NULL | ||
|
|
There was a problem hiding this comment.
Should these be moved to window.slt? On the other hand there are already other window function tests at aggregate.slt, so I'm not sure what is the criteria.
There was a problem hiding this comment.
Thanks @nuno-faria , and thanks for checking the predicate behavior against DuckDB/Postgres too.
I put these in aggregate.slt because the failure is in aggregate FILTER handling and there are already related aggregate/window aggregate cases here. So, Keeping the new cases alongside the existing aggregate FILTER coverage felt more consistent to me.
Which issue does this PR close?
Rationale for this change
Aggregate and window aggregate
FILTERclauses currently fail with an internal error when the filter condition isNULL, even though the equivalent boolean-typed expression (e.g.CAST(NULL AS BOOLEAN)) works correctly.This occurs because
FILTERpredicates are not being coerced toBOOLEANduring type coercion.What changes are included in this PR?
FILTERpredicates toBOOLEANduring type coercion.FILTERpredicates.FILTER (WHERE NULL)for both aggregate and window aggregate functions.Are these changes tested?
Yes.
Added SQL logic tests covering:
COUNT(*) FILTER (WHERE NULL)COUNT(1) FILTER (WHERE NULL)SUM(1) FILTER (WHERE NULL)AVG(1) FILTER (WHERE NULL)FILTER (WHERE NULL)Are there any user-facing changes?
Yes.
Queries using
FILTER (WHERE NULL)no longer fail with an internal error and now return the expected results. Also, no changes were made to any public APIs