apache / apache/datafusion

Centralize Aggregate FILTER Some(true) Semantics for Consistency and Correctness Hardening

Open
#22,665 0 comments 0 reactions 1 assignee Claimed by @kosiew View on GitHub
Dominant language
Rust
Stars
9.3k
Forks
2.4k
Avg merge
3d 7h
Merged PRs (30d)
344

Description

## Summary
Aggregate FILTER row-validity handling is currently fragmented across helper and grouped accumulator paths. We should centralize the invariant that a row passes aggregate FILTER if and only if the predicate is Some(true).

## Current State
- `filter_to_validity` was introduced and grouped-accumulator helper paths were updated (including `accumulate_multiple` and `filter_to_nulls` plumbing) in #22068
- `first_last.rs` still uses local filter checks in grouped paths.
- `accumulate_indices` still repeats 64-bit validity iteration logic across multiple branches.

## Problem
Duplicated nullable-filter logic increases the risk of semantic drift and regressions (especially around NULL predicate handling), and makes validity bitmap code harder to maintain.

Because grouped and non-grouped paths can evolve independently, this drift can become user-visible as incorrect aggregate results under nullable predicates.

## Proposal
1. Add one shared helper/iterator in `functions-aggregate-common` that encodes:
- row passes FILTER iff predicate is `Some(true)`
2. Migrate grouped `first_value` / `last_value` filter paths to this shared utility.
3. Refactor `accumulate_indices` to reuse shared validity iteration and remove duplicated loops.
4. Migrate remaining grouped accumulators incrementally.

## Scope
In scope:
- Shared FILTER validity/predicate utility.
- Targeted migration of `first_last` grouped paths.
- Targeted cleanup in `accumulate_indices`.
- Focused test coverage for nullable FILTER predicates.

Out of scope:
- Broad performance rewrites not tied to FILTER semantics.
- Large cross-workspace refactors.

## Acceptance Criteria
1. A single reusable utility encodes FILTER pass criteria as `Some(true)`.
2. Grouped `first_value` / `last_value` no longer use bespoke nullable filter checks.
3. `accumulate_indices` no longer carries duplicated validity loop logic that can drift semantically.
4. Existing aggregate FILTER tests pass.
5. New tests cover nullable FILTER predicates for grouped first/last behavior.
6. No regression for non-null filter predicates.

## Testing
- Add SQLLogicTests for grouped `first_value` and `last_value` with nullable FILTER predicates.
- Include cases where value bits may be true while validity is NULL.
- Validate result is NULL when no rows satisfy `Some(true)`.
- Run relevant aggregate crate tests and sqllogictest aggregate coverage.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.