diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 6eeeb0e1d03..72a26695f06 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -3280,10 +3280,53 @@ adjust_group_pathkeys_for_groupagg(PlannerInfo *root) if (AGGKIND_IS_ORDERED_SET(aggref->aggkind)) continue; - /* only add aggregates with a DISTINCT or ORDER BY */ - if (aggref->aggdistinct != NIL || aggref->aggorder != NIL) - unprocessed_aggs = bms_add_member(unprocessed_aggs, - foreach_current_index(lc)); + /* Skip unless there's a DISTINCT or ORDER BY clause */ + if (aggref->aggdistinct == NIL && aggref->aggorder == NIL) + continue; + + /* Additional safety checks are needed if there's a FILTER clause */ + if (aggref->aggfilter != NULL) + { + ListCell *lc2; + bool allow_presort = true; + + /* + * When the Aggref has a FILTER clause, it's possible that the + * filter removes rows that cannot be sorted because the + * expression to sort by results in an error during its + * evaluation. This is a problem for presorting as that happens + * before the FILTER, whereas without presorting, the Aggregate + * node will apply the FILTER *before* sorting. So that we never + * try to sort anything that might error, here we aim to skip over + * any Aggrefs with arguments with expressions which, when + * evaluated, could cause an ERROR. Vars and Consts are ok. There + * may be more cases that should be allowed, but more thought + * needs to be given. Err on the side of caution. + */ + foreach(lc2, aggref->args) + { + TargetEntry *tle = (TargetEntry *) lfirst(lc2); + Expr *expr = tle->expr; + + while (IsA(expr, RelabelType)) + expr = (Expr *) (castNode(RelabelType, expr))->arg; + + /* Common case, Vars and Consts are ok */ + if (IsA(expr, Var) || IsA(expr, Const)) + continue; + + /* Unsupported. Don't try to presort for this Aggref */ + allow_presort = false; + break; + } + + /* Skip unsupported Aggrefs */ + if (!allow_presort) + continue; + } + + unprocessed_aggs = bms_add_member(unprocessed_aggs, + foreach_current_index(lc)); } /* diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index 30af83f23cc..9b97fcf40b5 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -1586,6 +1586,43 @@ select sum(two order by two) from tenk1; (2 rows) reset enable_presorted_aggregate; +-- +-- Test cases with FILTER clause +-- +-- Ensure we presort when the aggregate contains plain Vars +explain (costs off) +select sum(two order by two) filter (where two > 1) from tenk1; + QUERY PLAN +------------------------------- + Aggregate + -> Sort + Sort Key: two + -> Seq Scan on tenk1 +(4 rows) + +-- Ensure we presort for RelabelType'd Vars +explain (costs off) +select string_agg(distinct f1, ',') filter (where length(f1) > 1) +from varchar_tbl; + QUERY PLAN +------------------------------------- + Aggregate + -> Sort + Sort Key: f1 + -> Seq Scan on varchar_tbl +(4 rows) + +-- Ensure we don't presort when the aggregate's argument contains an +-- explicit cast. +explain (costs off) +select string_agg(distinct f1::varchar(2), ',') filter (where length(f1) > 1) +from varchar_tbl; + QUERY PLAN +------------------------------- + Aggregate + -> Seq Scan on varchar_tbl +(2 rows) + -- -- Test combinations of DISTINCT and/or ORDER BY -- diff --git a/src/test/regress/expected/sqljson.out b/src/test/regress/expected/sqljson.out index 67b68dfc232..9e52914a182 100644 --- a/src/test/regress/expected/sqljson.out +++ b/src/test/regress/expected/sqljson.out @@ -835,22 +835,22 @@ SELECT FROM (VALUES (NULL), (3), (1), (NULL), (NULL), (5), (2), (4), (NULL)) foo(bar); -[ RECORD 1 ]--------------------+------------------------------------------------------------------------------------------------------------------------- -no_options | [1, 2, 3, 4, 5] -returning_jsonb | [1, 2, 3, 4, 5] -absent_on_null | [1, 2, 3, 4, 5] -absentonnull_returning_jsonb | [1, 2, 3, 4, 5] -null_on_null | [1, 2, 3, 4, 5, null, null, null, null] -nullonnull_returning_jsonb | [1, 2, 3, 4, 5, null, null, null, null] -row_no_options | [{"bar":1}, + - | {"bar":2}, + +no_options | [3, 1, 5, 2, 4] +returning_jsonb | [3, 1, 5, 2, 4] +absent_on_null | [3, 1, 5, 2, 4] +absentonnull_returning_jsonb | [3, 1, 5, 2, 4] +null_on_null | [null, 3, 1, null, null, 5, 2, 4, null] +nullonnull_returning_jsonb | [null, 3, 1, null, null, 5, 2, 4, null] +row_no_options | [{"bar":null}, + | {"bar":3}, + - | {"bar":4}, + + | {"bar":1}, + + | {"bar":null}, + + | {"bar":null}, + | {"bar":5}, + - | {"bar":null}, + - | {"bar":null}, + - | {"bar":null}, + + | {"bar":2}, + + | {"bar":4}, + | {"bar":null}] -row_returning_jsonb | [{"bar": 1}, {"bar": 2}, {"bar": 3}, {"bar": 4}, {"bar": 5}, {"bar": null}, {"bar": null}, {"bar": null}, {"bar": null}] +row_returning_jsonb | [{"bar": null}, {"bar": 3}, {"bar": 1}, {"bar": null}, {"bar": null}, {"bar": 5}, {"bar": 2}, {"bar": 4}, {"bar": null}] row_filtered_agg | [{"bar":3}, + | {"bar":4}, + | {"bar":5}] diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql index 675b23add59..fe4d89aec6a 100644 --- a/src/test/regress/sql/aggregates.sql +++ b/src/test/regress/sql/aggregates.sql @@ -595,6 +595,25 @@ explain (costs off) select sum(two order by two) from tenk1; reset enable_presorted_aggregate; +-- +-- Test cases with FILTER clause +-- + +-- Ensure we presort when the aggregate contains plain Vars +explain (costs off) +select sum(two order by two) filter (where two > 1) from tenk1; + +-- Ensure we presort for RelabelType'd Vars +explain (costs off) +select string_agg(distinct f1, ',') filter (where length(f1) > 1) +from varchar_tbl; + +-- Ensure we don't presort when the aggregate's argument contains an +-- explicit cast. +explain (costs off) +select string_agg(distinct f1::varchar(2), ',') filter (where length(f1) > 1) +from varchar_tbl; + -- -- Test combinations of DISTINCT and/or ORDER BY --