fix: grouped first_value/last_value FILTER excludes NULL predicate rows

[u70b3]

Which issue does this PR close?

• Closes Grouped first_value/last_value FILTER Incorrectly Includes NULL Predicate Rows #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:

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 BooleanArrays 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.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 88.23529% with 8 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@39e2f23). Learn more about missing BASE report.
⚠️ Report is 31 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/functions-aggregate/src/first_last.rs	88.23%	3 Missing and 5 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #23707   +/-   ##
=======================================
  Coverage        ?   80.75%           
=======================================
  Files           ?     1089           
  Lines           ?   368844           
  Branches        ?   368844           
=======================================
  Hits            ?   297863           
  Misses          ?    53220           
  Partials        ?    17761           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[kosiew]

@u70b3
Looks good overall. I left one non-blocking suggestion to strengthen the test coverage around sliced nullable BooleanArrays.

[kosiew]

Could we also add a case where the nullable BooleanArray is sliced to a non-zero offset before calling update_batch? That would help explicitly cover the bitmap-offset boundary involved in the new validity and value checks. The current implementation already uses Arrow's offset-aware accessors correctly, so this is only a test-hardening suggestion.

[u70b3]

Fixed, thanks! Added coverage using a sliced nullable BooleanArray with a non-zero offset.