MDEV-24943: Implement FILTER clause support for aggregate functions

[KhaledR57]

• The Jira issue number for this PR is: MDEV-24943

Description

Aggregates lacked the SQL-standard FILTER clause, forcing CASE-based workarounds that reduced readability.

This update introduces the ability to specify a FILTER clause for aggregate functions, allowing for more granular control over which rows are included in the aggregation. Also, improves standards compliance and makes queries clearer and more readable.

<aggregate_function> ( <expression> ) [ FILTER ( WHERE <condition> ) ] [ OVER ( <window_spec> ) ]

The <condition> may contain any expression allowed in regular WHERE clauses, except subqueries, window functions, and outer references.

Example:

-- Using FILTER clause
SELECT AVG(value) FILTER (WHERE status = 'active') FROM sales;

-- Equivalent using CASE
SELECT AVG(CASE WHEN status = 'active' THEN value END) FROM sales;

How can this PR be tested?

Running the Test Suite

./mtr aggregates-filter

Basing the PR against the correct MariaDB version

• This is a new feature or a refactoring, and the PR is based against the main branch.
• This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

• I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
• For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

[gkodinov]

Thank for taking the effort to work on this. Indeed, the LIMIT clause is a part of (at least) SQL 2011 and, as such, is a very good feature to have.

However, the diff seems very incomplete. Can you please make sure that you:

1. Have a working diff.
2. Have a good functional description of the change (hopefully in a jira)

Please re-submit when you have the above.
This is not a trivial change. I would suggest reaching out to the mariaDB developers at zulip if you need to talk about it with somebody.

[vuvova]

I don't see how the diff is incomplete. It applies fine. It compiles fine. It passes tests on buildbot, not every builder, but it passes on many builders, this doesn't look like a non-working diff.

[DaveGosselin-MariaDB]

Hi @KhaledR57 , I'm experimenting with the changes and I see a difference between what should otherwise be equivalent statements. What was once written using CASE as a workaround should now work with FILTER as in the following examples below. First, on Postgres 18.1, we see that these queries are equivalent:

postgres=# SELECT COUNT(*) AS unfiltered, SUM( CASE WHEN generate_series < 5 THEN 1 ELSE 0 END ) AS filtered FROM generate_series(1,10);
 unfiltered | filtered
------------+----------
         10 |        4
(1 row)

postgres=# SELECT COUNT(*) AS unfiltered, COUNT(*) FILTER (WHERE generate_series < 5) AS filtered FROM generate_series(1,10);
 unfiltered | filtered
------------+----------
         10 |        4
(1 row)

However, on MariaDB, the FILTER analogue produces different results with your patch:

MariaDB [test]> SELECT COUNT(*) AS unfiltered, SUM( CASE WHEN seq < 5 THEN 1 ELSE 0 END ) AS filtered FROM seq_1_to_10;
+------------+----------+
| unfiltered | filtered |
+------------+----------+
|         10 |        4 |
+------------+----------+
1 row in set (0.005 sec)

MariaDB [test]> SELECT COUNT(*) AS unfiltered, COUNT(*) FILTER (WHERE seq < 5) AS filtered FROM seq_1_to_10;
+------------+----------+
| unfiltered | filtered |
+------------+----------+
|         10 |       10 |
+------------+----------+
1 row in set (0.001 sec)

Why is this the case?

[DaveGosselin-MariaDB]

If we store the sequence engine result to an InnoDB-backed table and run the FILTER query against that instead we get the correct result:

create table t1 (seq int);
insert into t1 select seq from seq_1_to_10;
SELECT COUNT(*) AS unfiltered, COUNT(*) FILTER (WHERE seq < 5) AS filtered FROM t1;
+------------+----------+
| unfiltered | filtered |
+------------+----------+
|         10 |        4 |
+------------+----------+
1 row in set (0.001 sec)

[KhaledR57]

Hi @DaveGosselin-MariaDB , Sorry for the delayed response!

The issue was with the sequence storage engine's group_by_handler optimization in storage/sequence/sequence.cc. It computes SUM() and COUNT() directly using formulas without scanning rows, but it wasn't checking for the FILTER clause, so the filter was completely bypassed.

I've added a has_filter() check so it falls back to normal aggregation when FILTER is present. Fixed now!

if (item->type() != Item::SUM_FUNC_ITEM ||
    (((Item_sum*) item)->sum_func() != Item_sum::SUM_FUNC &&
     ((Item_sum*) item)->sum_func() != Item_sum::COUNT_FUNC) ||
    ((Item_sum*) item)->has_filter())  // NEW CHECK
    return 0;  // Fall back to normal aggregation

Storing the sequence data to an InnoDB table gave the correct result because InnoDB goes through the standard opt_sum.cc optimization path, which I had already updated here to check has_filter() and skip the optimization when FILTER is present.

I'll add sequence-specific tests in the next commit after I finish the stored aggregates (almost done with those).

[DaveGosselin-MariaDB]

Hi @KhaledR57 ,

I've added a has_filter() check so it falls back to normal aggregation when FILTER is present. Fixed now!

Will such a change be required for every storage engine? If so, is there a way to generalize this for every storage engine?

[DaveGosselin-MariaDB]

Hi @KhaledR57 ,
Here are the places I found in your patch which are not exercised by your new tests. Please add test cases that exercise them. Feel free to reach out to me on Zulip if you need help. I still have more review work to do and will update again soon.
Thanks,
Dave

[KhaledR57]

Hi @KhaledR57 ,

I've added a has_filter() check so it falls back to normal aggregation when FILTER is present. Fixed now!

Will such a change be required for every storage engine? If so, is there a way to generalize this for every storage engine?

Hi @DaveGosselin-MariaDB,
I actually had the same question. This issue made me realize other engines with similar optimizations might have the same problem.
I was focused on the JOIN bug with FILTER clauses and custom aggregates, so I didn’t get to generalize this yet. I plan to test other storage engines to ensure FILTER clauses works correctly.

[DaveGosselin-MariaDB]

Hi @KhaledR57 ,
In case you didn't notice, please see this comment on the associated Jira ticket.
Thanks,
Dave

[KhaledR57]

Hi @KhaledR57 , In case you didn't notice, please see this comment on the associated Jira ticket. Thanks, Dave

Sorry, I wasn't following the ticket. I believe these are already handled.
In opt_sum_query for optimization opt_sum.cc, and for QUICK_GROUP_MIN_MAX_SELECT opt_range.cc

Or the comment meant something else. If I misunderstood something, please let me know.

[DaveGosselin-MariaDB]

Hi @KhaledR57 , thanks for your patience. I'm still working through some test cases and looking at the code. Please see my latest comments here.

[DaveGosselin-MariaDB]

Things are looking pretty good, just a couple of observations and questions for you to look into.

I think it's time to add the HTON_ flag (see handler.h) support that you, Sergei, and I discussed over on Zulip. We don't want to introduce silent 'wrong result' issues for unsupported engines. You can look at how existing HTON_ flags are used and feel free to ping me with any specific questions.

[DaveGosselin-MariaDB]

If we force new_field to be NULL here (with a debugger) and let the program continue, then we trigger the assertion DBUG_ASSERT(tab->join || current_thd->is_error()); at sql_select.cc:12956. The stack trace is

* frame #4: 0x0000000100642d08 mariadbd`next_breadth_first_tab(first_top_tab=0x000000012002a718, n_top_tabs_count=2, tab=0x000000012002b008) at sql_select.cc:12956:3
    frame #5: 0x00000001006353f0 mariadbd`JOIN::cleanup(this=0x000000013535dd28, full=true) at sql_select.cc:17289:19
    frame #6: 0x0000000100635008 mariadbd`JOIN::destroy(this=0x000000013535dd28) at sql_select.cc:5123:3
    frame #7: 0x000000010070dd6c mariadbd`st_select_lex::cleanup(this=0x0000000135358410) at sql_union.cc:2975:18
    frame #8: 0x000000010060cec4 mariadbd`mysql_select(thd=0x00000001401d8088, tables=0x000000013535b610, fields=0x00000001353586c8, conds=0x0000000000000000, og_num=1, order=0x0000000000000000, group=0x000000013535d350, having=0x0000000000000000, proc_param=0x0000000000000000, select_options=2164525824, result=0x000000013535dd00, unit=0x00000001401dc6a8, select_lex=0x0000000135358410) at sql_select.cc:5428:29

This is likely a pre-existing problem because other places in the same function also goto err;. Building as a Release build instead of Debug doesn't result in a crash, but some test results are different if the error is forced (when I think it would be better to emit an error).

[DaveGosselin-MariaDB]

Noting that, with this commit (01f25f9059f5) rebased to the latest main (df64bf8114a7b), this crash still occurs (if we force the new_field = nullptr case with a debugger) with the same assertion and stack trace. This isn't strictly due to the current patch, but deserves its own Jira ticket. We claim to handle the OOM case originating from Create_tmp_table::add_fields but this demonstrates that we don't (at least in Debug builds).

Place a breakpoint in the following, found in Create_tmp_table::add_fields:

          Field *new_field=
            create_tmp_field(table, arg, &copy_func,
                             tmp_from_field, &m_default_field[fieldnr],
                             m_group != 0, not_all_columns,
                             distinct_record_structure , false);
          if (!new_field) /// <--- breakpoint here, set new_field to NULL, then continue
            goto err;					// Should be OOM

The repro case with FILTER is (I haven't taken the time to simplify it):

CREATE TABLE test_aggregates (
  id INT PRIMARY KEY,
  category VARCHAR(50),
  status VARCHAR(20),
  value INT,
  price DECIMAL(10,2),
  amount DECIMAL(10,2) unique NOT NULL,
  name VARCHAR(50),
  key_name VARCHAR(50),
  value_col VARCHAR(50),
  bit_value INT,
  extra_value float(10,2),
  geom GEOMETRY
);

CREATE TABLE test_aggregates2 (
  id INT PRIMARY KEY,
  ref_id INT,
  extra_value INT
);

delimiter |
CREATE AGGREGATE FUNCTION weighted_avg(val INT, weight INT) RETURNS DOUBLE
BEGIN
  DECLARE sum_val_weight DOUBLE DEFAULT 0;
  DECLARE sum_weight DOUBLE DEFAULT 0;
  DECLARE CONTINUE HANDLER FOR NOT FOUND
    RETURN IF(sum_weight > 0, sum_val_weight / sum_weight, NULL);
  LOOP
    FETCH GROUP NEXT ROW;
    SET sum_val_weight = sum_val_weight + val * weight;
    SET sum_weight = sum_weight + weight;
  END LOOP;
END|
delimiter ;

INSERT INTO test_aggregates2 VALUES
(1, 1, 10),
(2, 2, 20),
(3, 3, 30),
(4, 4, 40);

SELECT 
    t1.category,
    AVG(t1.value) FILTER (WHERE t2.extra_value > 15) as avg_result,
    weighted_avg(t1.value, t1.amount) FILTER (WHERE t2.extra_value > 15) as weighted_avg_result,
    SUM(t1.value) FILTER (WHERE t2.extra_value > 20) as sum_result,
    COUNT(*) FILTER (WHERE t2.extra_value > 25) as count_result
FROM test_aggregates t1
JOIN test_aggregates2 t2 ON t1.id = t2.ref_id
GROUP BY t1.category;

[DaveGosselin-MariaDB]

(same if we force this branch)

[DaveGosselin-MariaDB]

(Noting that, with this commit (01f25f9) rebased to the latest main (df64bf8), this crash still occurs (if we force the new_field = nullptr case with a debugger) with the same assertion and stack trace, same as above).

[gkodinov]

This is a preliminary review to get some of the basics right.

Please squash your commits in a single one and update the commit message.

Otherwise, nothing to add to David's comments. Please keep working with him.

[KhaledR57]

Hi @gkodinov Sorry for the delay, I was away for a while. I am back now and have updated the PR

[DaveGosselin-MariaDB]

@mariadb-YuchenPei I have looked through the latest squashed commit and am currently looking through the Claude review feedback. The Spider change looks simple, can you please review it, it appears confined to storage/spider/spd_db_mysql.cc. Thanks in advance.

[DaveGosselin-MariaDB]

@KhaledR57 below is feedback from an automated review by Claude. It appears to me that these are legitimate issues that we need to address. Please look over them and let me know what you think. Claude refers to itself as "I" below, so those remarks are from it rather than from me.

Separately, in the GitHub description, why is subqueries crossed-out? If the patch now supports subqueries, then please update the description accordingly.

Begin Claude Review of: MDEV-24943 FILTER clause for aggregate functions

I found a number of issues that need to be resolved before this can go in. The
most important thing to say up front is that four of them are recorded as
expected output in mysql-test/main/aggregates-filter.result, so the test suite
currently passes while blessing wrong answers. Please treat those as the first
priority, since re-recording the result file would cement them.

Everything below was reproduced against a debug build of this branch unless
marked otherwise.

---

Wrong results

1. SUM and AVG over FLOAT or DOUBLE ignore the filter (grouped path)

sql/item_sum.cc:2954, Item_sum_sum::reset_field()

The REAL branch stores the argument into the group accumulator before the filter
is consulted:

    double nr= likely(!direct_added) ? args[0]->val_real() : direct_sum_real;
    float8store(result_field->ptr, nr);        // stored unconditionally
  }
  ...
  else
    null_flag= args[0]->null_value || !filter_passed();

The filter only controls the NULL bit. Item_sum_sum::update_field() at
sql/item_sum.cc:3096 then reads that value back with float8get(old_nr, res)
and adds to it, and unlike the DECIMAL branch at line 3077 it has no
result_field->is_null() guard. So the first row of every group is included
even when the filter rejects it.

This one is in the recorded output. aggregates-filter.test:208 runs

SELECT category, SUM(extra_value) FILTER (WHERE extra_value > 10)
FROM test_aggregates GROUP BY category;

The Electronics rows hold extra_value values 10.00, 25.00 and 30.00. Since
10.00 > 10 is false, the answer is 55.00, but aggregates-filter.result:54
records Electronics 65.00. The excluded row is being summed.

The DECIMAL path is correct, which is why this went unnoticed; every other
SUM/AVG column in the test is INT or DECIMAL.

2. Same bug on the streaming path

sql/item_sum.cc:1773, Item_sum_sum::add_helper()

      if (perform_removal && count > 0)
        sum-= aggr->arg_val_real();
      else
        sum+= aggr->arg_val_real();          // accumulated before the check
      if (!aggr->arg_is_null(true) && filter_passed())

sum is accumulated unconditionally; only count and null_value are gated.
The existing code was correct for NULLs only because arg_val_real() returns
0.0 for NULL. A row that the filter rejects contributes its full value.

CREATE TABLE t1 (i INT, d DOUBLE);
INSERT INTO t1 VALUES (1,100.5),(1,4),(2,31);
SELECT SUM(d) FILTER (WHERE i=1) FROM t1;   -- 135.5, should be 104.5

This reaches the query with no GROUP BY, WITH ROLLUP, SQL_BIG_RESULT, and
window frames. AVG is worse, because Item_sum_avg::count is filtered while
the inherited sum is not, so AVG can come out larger than MAX.

3. JSON_ARRAYAGG emits null for excluded rows instead of dropping them

sql/item_sum.cc:4294 and 4316, Item_func_group_concat::add()

  if ((always_null || !filter_passed()) && exclude_nulls)
    return 0;
  ...
      if (!filter_passed() && !exclude_nulls)
        field->set_null();

Item_func_json_arrayagg::skip_nulls() returns false, so exclude_nulls is
false and the early return never fires. The row is still inserted into the tree
with its field forced to NULL.

-- rows (1,'x'),(0,'y'),(1,'z')
SELECT JSON_ARRAYAGG(c) FILTER (WHERE b=1) FROM t1;
-- ["x",null,"z"], should be ["x","z"]

Also recorded. aggregates-filter.result:11 has
["Phone","Tablet",null,null,"Laptop",null,null,null] where the correct answer
is ["Tablet","Laptop"], and lines 36 and 37 record [null,null,null] and
[null,null] for groups where every row is excluded, where the answer should be
NULL.

GROUP_CONCAT and JSON_OBJECTAGG are correct on the same data, which is a
useful cross check.

One more thing on this line. Field::set_null() is a no-op when null_ptr is
0, so for JSON_ARRAYAGG over a NOT NULL column the excluded row's actual
value ends up in the array.

4. Stored aggregate returns the previous group's value

sql/item_sum.cc:1509 and 1518

Item_sum_sp::add() returns early without calling execute_impl(), so a group
where the filter excludes every row never creates func_ctx. The guard added
to clear() then skips the state reset:

void Item_sum_sp::clear()
{
  if (!func_ctx)
    return;                        // skips sp_query_arena->free_items()

aggregates-filter.test:215 runs weighted_avg(value, amount) FILTER (WHERE value > 0) grouped by category and status. Books/inactive is the single row
with value NULL, so nothing is aggregated and the NOT FOUND handler must
return NULL. aggregates-filter.result:66 records 200, which is exactly the
preceding Books/active result. The AVG(value) column of the same query
correctly records NULL on that row, so the result file contradicts itself.

It is worse at aggregates-filter.result:393-399, where four of six rows are
wrong and the recorded value 150 is not derivable from any Books row at all.

Separately, when a later group does match, the value fetch builds a fresh
context and runs one body iteration against whatever is in record[0]. That
means a row the user explicitly excluded is passed to their stored function
body.

The if (!func_ctx) guard is treating a symptom. The underlying problem is
that a filtered group never initializes the context at all, so add() needs to
either establish the context anyway or the empty-group case needs an explicit
path.

---

Crashes

5. UDF aggregates accept FILTER but were never wired up

sql/item_sum.h:1751, Item_udf_sum::fix_fields

fix_filter(thd) was added to five fix_fields overrides. Item_udf_sum is
the sixth and was missed. Item_udf_sum::add() and remove() never call
filter_passed(), and Item_udf_sum::print() does not emit the clause.

The grammar reaches it. sql_yacc.yy:11445 attaches the filter to anything
whose type() is SUM_FUNC_ITEM, and Create_udf_func builds Item_sum_udf_*
for UDFTYPE_AGGREGATE.

CREATE AGGREGATE FUNCTION avgcost RETURNS REAL SONAME "udf_example.so";
CREATE TABLE t1 (g INT, qty INT, price DOUBLE);
SELECT avgcost(qty, price) FILTER (WHERE g = 1) FROM t1;   -- SIGSEGV

Backtrace runs Item_udf_sum::fix_fields into Item_sum::check_sum_func into
the new Item_sum::update_used_tables at sql/item_sum.cc:579, which
dereferences a filter that was never fixed.

Either add fix_filter() and the filter_passed() checks, or reject FILTER on
UDF aggregates with a clear error. Silently ignoring it is the one option that
is not acceptable, because a view over such a query also loses the clause.

6. AVG evaluates the filter twice, so the sum and the count diverge

sql/item_sum.cc:2127

bool Item_sum_avg::add()
{
  if (Item_sum_sum::add())                            // consults the filter
    return TRUE;
  if (!aggr->arg_is_null(true) && filter_passed())    // consults it again
    count++;

add_helper() already ran the predicate to decide sum; AVG runs it again to
decide count. That is 12 evaluations for a 6 row table. Any predicate that
is not stable across two evaluations makes the numerator and the denominator
cover different row sets.

SET @n=0;
SELECT AVG(i) FILTER (WHERE (@n:=@n+1) MOD 2 = 0) FROM t1;

This aborts the server:

Assertion failed: ((m_ptr == __null) == item->null_value),
file sql_type.cc, line 335

count reaches 6 while Item_sum_sum::null_value is still 1, so
Item_sum_avg::val_decimal() passes its if (!count) guard and returns a
non-NULL pointer with null_value set.

A non-crashing form of the same problem:

SET @n=0, @m=0;
SELECT AVG(i) FILTER (WHERE (@n:=@n+1)<=5),
       SUM(i) FILTER (WHERE (@m:=@m+1)<=5) FROM t1;
-- AVG=30.0000 with SUM=150; AVG summed three rows but counted two

It is also plan dependent. The tmp table path evaluates the predicate once per
row, so the same query answers differently depending on whether the optimizer
picks end_send_group or end_update.

Item_sum_avg::remove() at line 2135 has the same shape.

7. Item_sum_sum::update_field() re-tests the filter and can discard the running total

sql/item_sum.cc:3072 and 3077

      null_flag= args[0]->null_value || !filter_passed();   // first evaluation
    }
    if (!null_flag)
    {
      if (!result_field->is_null() && filter_passed())      // second evaluation

null_flag already encodes the filter, so the second call is redundant on the
normal path. The guard that was there, !result_field->is_null(), meant
exactly one thing, that the accumulator already holds a value and we should add
rather than store. Folding the filter into it conflates two unrelated
conditions, and when the second evaluation disagrees with the first, the else
branch at line 3083 replaces the group's accumulated sum with a single row's
value.

Observable today, since the DOUBLE and DECIMAL paths disagree on identical data
and an identical predicate:

SET @c=0; SELECT g, SUM(f) FILTER (WHERE (@c:=@c+1) MOD 2 = 1) FROM t1 GROUP BY g;  -- 5
SET @c=0; SELECT g, SUM(d) FILTER (WHERE (@c:=@c+1) MOD 2 = 1) FROM t1 GROUP BY g;  -- 8

Please drop the && filter_passed() at line 3077.

---

Backward compatibility

8. filter no longer works as an implicit column alias after a function call

sql/lex.h:251, with %nonassoc FILTER_SYM at sql_yacc.yy:1268 and the
%prec SUBQUERY_AS_EXPR on the empty opt_filter_expr production at
sql_yacc.yy:11648

These four all parsed before this change and are now syntax errors:

SELECT ABS(1) filter FROM t1;
SELECT CONCAT(a,b) filter FROM t1;
SELECT SUM(a) filter FROM t1;
SELECT COUNT(*) filter, 1 FROM t1;

The keyword list placement is right; AS filter, SELECT a filter, FROM t filter, CREATE TABLE t (filter INT) and DECLARE filter INT all still work.
The break comes from the precedence declarations resolving the new conflict as a
shift.

Worth knowing while you look at this. Rebuilding the grammar with those two
precedence declarations removed reports 74 shift/reduce conflicts against the
unchanged %expect 72. So the change does introduce two conflicts and the
precedence declarations hide them from the guard that exists to catch exactly
that. Please work out a formulation that does not need them, and add a test
using filter as an identifier in every position.

---

Feature gaps

9. FILTER is rejected on schema-qualified function calls

sql/sql_yacc.yy:11496

opt_filter_expr was added only to the unqualified ident_cli_func production
at line 11358. The two-part form at line 11496 and the three-part form at line
11518 were not touched.

SELECT agg1(a) FILTER (WHERE b=1) FROM t1;         -- works
SELECT test.agg1(a) FILTER (WHERE b=1) FROM t1;    -- ER_PARSE_ERROR

An aggregate stored function outside the default schema cannot be written with
FILTER at all.

10. A view over a cross-schema stored aggregate silently drops the clause

sql/item_func.h:4195

Item_func_sp::m_filter has a setter but no print() support. When the item
is printed before fix_fields converts it to Item_sum_sp, the clause
disappears.

USE d2;
CREATE VIEW test.v1 AS SELECT agg2(a) FILTER (WHERE b=1) AS s FROM test.t1;
SHOW CREATE VIEW test.v1;
-- select `agg2`(`test`.`t1`.`a`) AS `s` from `test`.`t1`      -- no FILTER

The view then computes an unfiltered aggregate, permanently, with no error. The
same view created inside d2 keeps the clause, and built-in SUM keeps it in
both cases.

11. print() emits the tmp table field rather than the predicate

sql/item_sum.cc:540, and the duplicate at line 4707

Arguments are deliberately protected from the tmp table substitution:

  Item **pargs= fixed() ? orig_args : args;

There is no orig_filter counterpart, so after
Create_tmp_table::add_fields rewrites the filter at sql/sql_select.cc:22377,
print() dumps the substituted field.

EXPLAIN EXTENDED SELECT SQL_BUFFER_RESULT SUM(a) FILTER (WHERE b=1) FROM t1;
SHOW WARNINGS;
-> select sql_buffer_result sum(`test`.`t1`.`a`) FILTER(WHERE `tmp_field`) ...

That is not valid SQL and the predicate is gone. The same text feeds the
optimizer trace. Views are unaffected because they print before optimization.

12. filter_passed() uses val_int() where boolean truth is val_bool()

sql/item_sum.cc:595

  return filter_expr->val_int();

val_int() rounds or truncates first. Type_handler_int_result::Item_val_bool
in sql/sql_type.cc:5324 carries an explicit comment that val_int() must not
be used to evaluate a condition.

CREATE TABLE t1 (a DOUBLE);
INSERT INTO t1 VALUES (0.4),(2.0);
SELECT COUNT(*) FILTER (WHERE a) FROM t1;   -- 1
SELECT COUNT(*) FROM t1 WHERE a;            -- 2

NULL handling happens to be right, since val_int() returns 0 for NULL and NULL
must exclude, but the two should agree in every case.

---

Structural suggestion

The filter is currently checked inside roughly 38 individual aggregate methods.
That is what let items 1, 2, 5 and the REAL branches slip through, and it is
going to keep happening as aggregates are added.

Every streaming add() and remove() reaches a row through one of three
places, Item_sum::aggregator_add(), Frame_cursor::add_value_to_items() and
Frame_cursor::remove_value_from_items() at sql/sql_window.cc:1176 and
1189. All eight update_field() sites reach it through
update_tmptable_sum_func() at sql/sql_select.cc:30352, and skipping
update_field() wholesale is already what the patch does for
Item_sum_min_max. Gating at those four points covers 25 of the 38 sites. The
nine reset_field() sites genuinely do need per class handling, since each has
to write its own empty-group encoding.

Doing it that way also removes the need for Aggregator::is_in_endup_phase().
That virtual exists on the shared Aggregator base only to undo a check that
sits below the DISTINCT collection and replay boundary; move the check above the
boundary and the replay path never sees it. Aggregator_distinct::add()
already gates collection, so the tree holds only rows that passed.

There is a measurable cost to the current shape as well. filter_passed() is
out of line in item_sum.cc and makes an indirect virtual call, so a plain
SELECT COUNT(*) FROM t over a large table pays a non-inlinable call per row
for a feature the query does not use. At minimum, make the fast path inline:

  bool filter_passed() { return !filter_expr || filter_passed_slow(); }

Two other duplication points while you are in there. The new block in
Create_tmp_table::add_fields at sql/sql_select.cc:22357 is a copy of the
argument block 40 lines above it, and it has already drifted (it omits the
FIELD_PART_OF_TMP_UNIQUE flag the original sets). And the FILTER(WHERE ...)
printing block is pasted into Item_sum::print() and
Item_func_group_concat::print() and missing from Item_udf_sum::print(); a
shared print_filter() helper would cover all three.

---

Test suite

The test file needs substantial rework.

The whole body from line 22 through 1094 runs under --disable_query_log, so
the 617 line result file contains no SQL at all. When one of the roughly 90
queries regresses, the diff shows a value change with no way to identify which
query produced it, which is how items 1, 3 and 4 above ended up recorded as
correct. Only 14 of the 1437 tests in mysql-test/main disable the log this
broadly. Please limit it to the noisy setup block.

Missing coverage, each item mapping to a defect above:

• No EXPLAIN anywhere, despite adding bail-outs to get_best_group_min_max()
  at opt_range.cc:14732 and opt_sum_query() at opt_sum.cc:386. The
  section at lines 924 to 996 is titled "loose index scan" and never checks a
  plan, so if either guard is later dropped the test still passes.
• No CREATE VIEW, SHOW CREATE VIEW or PREPARE, despite changing print()
  in two places.
• No UDF aggregate and no schema-qualified stored aggregate call.
• No use of filter as an identifier.
• One float(10,2) column and no true DOUBLE with SUM or AVG.
• No --error test for a subquery in FILTER, even though the commit message
  says subqueries are unsupported. They do in fact work, and lines 457 and 465
  rely on them, so the message needs correcting either way.
• No LEFT JOIN or RIGHT JOIN, so NULL-extended rows are untested.

Determinism. There are no --sorted_result directives, and only 6 of the ~90
queries carry an ORDER BY. Line 982 projects only a1 from GROUP BY a1,a2,b, so its 12 output rows are distinguishable by position alone; that is
the same section where loose index scan was just disabled, so a plan change
there is likely. ANALYZE TABLE output is recorded verbatim at result line 459
with no --replace_, and MyISAM can return Table is already up to date
instead of OK.

Naming. Please use t1, t2, t3 per section rather than test_aggregates,
test_aggregates2, empty_test and t1_min. The file already uses t1 and
t2 at lines 928 and 956, so it is inconsistent with itself.

---

Out of scope for this commit

sql/item_sum.cc:2717, Item_sum_bit::remove_as_window()

  if (num_values_added == 0 || args[0]->null_value || !filter_passed())

Adding args[0]->null_value changes results for existing window BIT_AND
queries over NULL data that use no FILTER clause at all:

CREATE TABLE t1 (pk INT PRIMARY KEY, b INT);
INSERT INTO t1 VALUES (1,NULL),(2,1),(3,2);
SELECT pk, BIT_AND(b) OVER (ORDER BY pk ROWS BETWEEN 1 PRECEDING AND CURRENT ROW)
FROM t1;

pk=3 returns 0 on this branch and 3 before it. The old path decremented
num_values_added for a NULL row that add() never counted, desynchronizing it
from bit_counters.

The new answer is the correct one, so this is a real fix, but it belongs in its
own commit with its own test. mysql-test/main/win_bit.test contains no NULL
data, and this commit changes no existing result file, so the fix currently
ships silently.

---

Smaller items

• sql/item_sum.cc:493. The Item_sum copy constructor returns on the
  arg_count > 2 allocation failure path before init_aggregator(), which is
  the only place filter_expr is set to NULL. aggr and with_distinct are
  already left uninitialized there, but filter_expr is worse because
  has_filter() then gates a dereference. A member initializer on the
  declaration would settle it.
• sql/sql_select.cc:22385. The new filter field omits if (current_counter == distinct) new_field->flags|= FIELD_PART_OF_TMP_UNIQUE; that the sibling block
  sets, while still doing m_field_count[current_counter]++ and add_field().
  I could not reach that branch with current_counter == distinct, so this is
  latent rather than live, but finalize() re-derives the bucket from that flag
  at line 22636, so the null bit accounting would collide the moment it becomes
  reachable.
• sql/item_sum.cc:599. fix_filter() does not merge filter_expr->with_flags
  into the aggregate, unlike the argument loop at line 1183. An aggregate whose
  filter holds a subquery reports with_subquery() false, and
  item_with_t::PARAM is lost for SUM(a) FILTER (WHERE b = ?).
• sql/item_sum.cc:3192. The early return in Item_sum_min_max::update_field()
  sits above the direct_added swap and its restore, so a filtered row leaves
  the flag set and the next row aggregates a stale direct_item. Latent only
  because this same commit gates Spider off at spd_db_mysql.cc:6105.
• Item_func_group_concat::add() evaluates the predicate 1 + arg_count_field
  times per row. The call at line 4294 is a pure side effect for
  JSON_ARRAYAGG, since && exclude_nulls discards its value. Reordering to
  if (exclude_nulls && (always_null || !filter_passed())) and hoisting one
  evaluation above the loop fixes both.
• count_field_types() at sql/sql_select.cc:29414 counts a filter column for
  every filtered aggregate, while add_fields only creates one under !m_group && !m_save_sum_fields. The mismatch over-allocates today, which is the safe
  direction, but the two predicates should be the same expression so a future
  change to either side cannot turn it into an under-count.
• Identical filter expressions on sibling aggregates each get their own tmp
  table column with no deduplication, so COUNT(*) FILTER (WHERE p), SUM(x) FILTER (WHERE p), AVG(x) FILTER (WHERE p) evaluates p three times per row
  and stores it three times.
• my_error(ER_WRONG_USAGE, MYF(0), "FILTER", "NON-AGGREGATE FUNCTION") at
  sql/item_func.cc:6916 and sql_yacc.yy:11462. All 31 existing
  ER_WRONG_USAGE call sites pass either a literal SQL keyword in caps or a
  lowercase descriptive phrase. Your own messages at item_sum.cc:609 and
  615 already use lowercase, so the feature currently emits both Incorrect usage of aggregate function and FILTER and Incorrect usage of FILTER and NON-AGGREGATE FUNCTION. Please use "non-aggregate function".

---

Style and commit message

git show <sha> --check reports trailing whitespace on 54 new lines, mostly in
the test file, plus a trailing blank line at end of file.

Five new lines exceed the 79 character limit:

sql/sql_yacc.yy:11358        89
sql/sql_select.cc:29414      87
sql/item_windowfunc.cc:177   82
sql/sql_select.cc:22354      81
sql/item_sum.h:367           80

sql/sql_select.cc:29414 also uses a // comment where the standard calls for
block form. sql/item_sum.cc:591 puts text on the /* and */ lines; the
other three comments the patch adds get this right, so it is inconsistent within
the change. There is one blank line rather than two between Item_sum::set_arg
and Item_sum::set_filter at sql/item_sum.cc:628.

The commit message needs a pass as well:

• The subject uses one space after MDEV-24943:; the convention is two.
• Body lines run to 136 characters against a 72 character wrap.
• except ~~subqueries~~, window functions, and outer references still has
  Markdown strikethrough, which reads as literal tildes in git log. The
  statement is also wrong; subqueries in FILTER work and the test relies on
  them.
• The message contains two squashed subjects. "Fix crash when first GROUP BY
  row is filtered by HAVING" should either be its own commit or be folded into
  the body as prose.

[gkodinov]

LGTM. Please keep working with the final reviewer.

[mariadb-YuchenPei]

Spider c++ change looks good, thanks, but please improve test coverage, see my other comments below

[mariadb-YuchenPei]

Nice, but please add test coverage for this, like so:

new file   storage/spider/mysql-test/spider/feature/r/pushdown_aggregate.result
@@ -0,0 +1,21 @@
+for master_1
+for child2
+for child3
+set spider_same_server_link= 1;
+CREATE SERVER srv FOREIGN DATA WRAPPER mysql
+OPTIONS (SOCKET "$MASTER_1_MYSOCK", DATABASE 'test',user 'root');
+create table t2 (c int);
+create table t1 (c int) ENGINE=Spider REMOTE_SERVER=srv REMOTE_TABLE=t2;
+insert into t1 values (3), (7), (4), (1), (5);
+explain
+select sum(c) filter (where c < 5) from t1;
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
+1	SIMPLE	t1	ALL	NULL	NULL	NULL	NULL	2	
+select sum(c) filter (where c < 5) from t1;
+sum(c) filter (where c < 5)
+8
+drop table t1, t2;
+drop server srv;
+for master_1
+for child2
+for child3
new file   storage/spider/mysql-test/spider/feature/t/pushdown_aggregate.test
@@ -0,0 +1,26 @@
+--disable_query_log
+--disable_result_log
+--source ../../t/test_init.inc
+--enable_result_log
+--enable_query_log
+--source ../../include/have_group_by_handler.inc
+
+set spider_same_server_link= 1;
+evalp CREATE SERVER srv FOREIGN DATA WRAPPER mysql
+OPTIONS (SOCKET "$MASTER_1_MYSOCK", DATABASE 'test',user 'root');
+create table t2 (c int);
+create table t1 (c int) ENGINE=Spider REMOTE_SERVER=srv REMOTE_TABLE=t2;
+
+insert into t1 values (3), (7), (4), (1), (5);
+
+explain
+select sum(c) filter (where c < 5) from t1;
+select sum(c) filter (where c < 5) from t1;
+
+drop table t1, t2;
+drop server srv;
+--disable_query_log
+--disable_result_log
+--source ../../t/test_deinit.inc
+--enable_result_log
+--enable_query_log

[KhaledR57]

@mariadb-YuchenPei what about columnstore, should I add the same has_filter() guard to columnstore, mirroring the spider fix?

[mariadb-YuchenPei]

Good question. I've asked the columnstore team. Let's wait and see what they say

[mariadb-YuchenPei]

Please add test coverage for the select handler, as mentioned in my previous comment #4439 (comment)

Something like this:

modified   mysql-test/suite/federated/federatedx_create_handlers.result
@@ -1577,6 +1577,38 @@ id	op	op_count
 drop table federated.t1, federated.t2;
 connection slave;
 drop table federated.t1, federated.t2;
+# End of 11.4 tests
+#
+# MDEV-24943: Implement FILTER clause support for aggregate functions
+#
+connection slave;
+CREATE TABLE federated.t1 (
+id int(20) NOT NULL,
+name varchar(16) NOT NULL default ''
+);
+INSERT INTO federated.t1 VALUES
+(3,'xxx'), (7,'yyy'), (4,'xxx'), (1,'zzz'), (5,'yyy');
+connection master;
+CREATE TABLE federated.t1 (
+id int(20) NOT NULL,
+name varchar(16) NOT NULL default ''
+)
+ENGINE="FEDERATED" DEFAULT CHARSET=latin1
+CONNECTION='mysql://root@127.0.0.1:19001/federated/t1';
+select sum(id) from federated.t1;
+sum(id)
+20
+explain
+select sum(id) filter (where name = 'yyy') from federated.t1;
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
+1	PUSHED SELECT	NULL	NULL	NULL	NULL	NULL	NULL	NULL	NULL
+select sum(id) filter (where name = 'yyy') from federated.t1;
+sum(id) filter (where name = 'yyy')
+12
+drop table federated.t1;
+connection slave;
+drop table federated.t1;
+# End of 13.2 tests
 connection default;
 set global federated_pushdown=0;
 connection master;
@@ -1585,4 +1617,3 @@ DROP DATABASE IF EXISTS federated;
 connection slave;
 DROP TABLE IF EXISTS federated.t1;
 DROP DATABASE IF EXISTS federated;
-# End of 11.4 tests
modified   mysql-test/suite/federated/federatedx_create_handlers.test
@@ -1024,9 +1024,43 @@ drop table federated.t1, federated.t2;
 connection slave;
 drop table federated.t1, federated.t2;
 
+--echo # End of 11.4 tests
+
+--echo #
+--echo # MDEV-24943: Implement FILTER clause support for aggregate functions
+--echo #
+
+--connection slave
+CREATE TABLE federated.t1 (
+  id int(20) NOT NULL,
+  name varchar(16) NOT NULL default ''
+);
+
+INSERT INTO federated.t1 VALUES
+  (3,'xxx'), (7,'yyy'), (4,'xxx'), (1,'zzz'), (5,'yyy');
+
+--connection master
+eval
+CREATE TABLE federated.t1 (
+  id int(20) NOT NULL,
+  name varchar(16) NOT NULL default ''
+)
+ENGINE="FEDERATED" DEFAULT CHARSET=latin1
+CONNECTION='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1';
+
+select sum(id) from federated.t1;
+explain
+select sum(id) filter (where name = 'yyy') from federated.t1;
+select sum(id) filter (where name = 'yyy') from federated.t1;
+
+drop table federated.t1;
+
+connection slave;
+drop table federated.t1;
+
+--echo # End of 13.2 tests
+
 # cleanup
 connection default;
 set global federated_pushdown=0;
 source include/federated_cleanup.inc;
-
---echo # End of 11.4 tests

[DaveGosselin-MariaDB]

(these changes actually go in the .test files, not the .result files but when you record the test they will then appear in both).

[DaveGosselin-MariaDB]

Please add a line to indicate the end of the tests for the version into which this will be added.

--echo End of AB.C tests

where AB.C is the version (e.g. 12.1 or something)

[DaveGosselin-MariaDB]

Please add a line to indicate the end of the tests for the version into which this will be added.

--echo End of AB.C tests  

where AB.C is the version (e.g. 12.1 or something)

[mariadb-YuchenPei]

Thanks for the update

[mariadb-YuchenPei]

Please move the header part to below the setup "--source ../../include/have_group_by_handler.inc"

[mariadb-YuchenPei]

The "end of test ..." block is optional. If you must have it, please place it after the end of the test case "drop server srv;" above

[mariadb-YuchenPei]

This should be placed before the teardown part, i.e. before the "--disable_query_log" above. If you keep the "end of test part ..." this should be after it that block because of the hierarchy of test > version > testcase

[mariadb-YuchenPei]

Good question. I've asked the columnstore team. Let's wait and see what they say