u70b3 opened a new pull request, #23707: URL: https://github.com/apache/datafusion/pull/23707
## Which issue does this PR close? - Closes #22666 ## Rationale for this change Under SQL aggregate `FILTER` semantics, a row passes only when the predicate evaluates to `true`; rows where the predicate is `null` must be excluded. Grouped `first_value` / `last_value` checked only `BooleanArray::value(idx)`, without checking validity, so a NULL predicate row whose underlying value bit is set (as produced by comparison kernels, e.g. `null::int < 1`) was treated as passing: ```sql SELECT g, first_value(a ORDER BY a) FILTER (WHERE b < 1) AS fv FROM (VALUES (0, 10, CAST(NULL AS INT)), (0, 20, 2)) AS t(g, a, b) GROUP BY g; -- returned fv = 10, must return fv = NULL -- (row 1: b < 1 is NULL; row 2: b < 1 is FALSE — no row satisfies `true`) ``` ## What changes are included in this PR? `datafusion/functions-aggregate/src/first_last.rs`, in `FirstLastGroupsAccumulator::get_filtered_extreme_of_each_group` (shared by `first_value` and `last_value`, and by both the `update_batch` and `merge_batch` paths): - `passed_filter` now requires `is_valid(idx) && value(idx)` (the `Some(true)` semantics), matching the convention already used by `variance.rs` / `correlation.rs`. - The `is_set_arr` read gets the same validity check. This is *not* only an internal bitmap: `convert_to_state` stores the user FILTER clause (including its nulls) in the last state column, so on the merge path (e.g. skip-partial-aggregation) NULL predicate rows were likewise treated as set. Verified with a forced skip-partial run (100k unique groups, all-NULL predicates): 83,616 groups were incorrectly assigned non-NULL values before the fix, 0 after. For genuine internal bitmaps (no nulls) the added check is trivially true, so behavior there is unchanged. Regression coverage: - sqllogictest (`aggregate.slt`): the issue reproducer, the `last_value` counterpart, mixed TRUE/FALSE/NULL predicates, all-TRUE and no-FILTER controls, the (already correct) non-grouped path, and a window-function no-regression case. - Unit tests: `test_group_acc_filter_null_predicate` (update path) and `test_group_acc_merge_null_is_set` (merge path via `convert_to_state` → `merge_batch`), both constructing `BooleanArray`s whose null slots carry a set value bit. ## Are these changes tested? Yes — see above. Verified `./dev/rust_lint.sh`, `cargo test -p datafusion-functions-aggregate --lib`, the `aggregate`/`window` sqllogictest files, and `datafusion-cli` end-to-end (grouped first/last_value now return NULL for the issue reproducer; mixed-predicate and non-grouped results unchanged). Also audited the rest of `functions-aggregate` for the same validity-blind pattern: shared helpers (`nulls.rs::filter_to_validity`, `accumulate.rs`, `prim_op.rs`, `count.rs`, `array_agg.rs`, `variance.rs`, `correlation.rs`) already handle validity correctly, and non-grouped paths pre-filter with arrow's `filter` kernel (which drops NULL predicate rows), so no other aggregate needs changes. ## Performance `functions-aggregate/benches/first_last.rs` was run against `main`. The added checks are one validity-bit test per row on the grouped path; measured deltas were within the machine's noise floor (±5% on unchanged `filter=false` cases). A variant hoisting the null check out of the row loop showed no measurable benefit beyond noise, so the simple idiomatic form is kept. ## Are there any user-facing changes? Only the bug fix: grouped `first_value`/`last_value` with a nullable `FILTER` predicate now correctly exclude NULL-predicate rows, matching SQL semantics and the behavior of other aggregates. No API or configuration changes. -- 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]
