Jackie-Jiang opened a new pull request, #19355:
URL: https://github.com/apache/pinot/pull/19355
## Summary
Follow-up cleanup to the null-contract work in #19218. Now that every
user-facing aggregation supports null handling, the option and the helpers for
reading only non-null rows no longer belong to a mixin that only single-input
functions could inherit.
**`NullSkippingUtils`** — the iteration helpers move out of
`NullableSingleInputAggregationFunction` into a static utility, so code that is
not an `AggregationFunction` at all can share them. The funnel aggregation
strategies were carrying their own copy of the walk; they now call the utility.
**`BaseAggregationFunction`** — a new base holding `_nullHandlingEnabled`
plus the convenience wrappers. `BaseSingleInputAggregationFunction` extends it,
so its 48 subclasses are unaffected beyond the repoint. Multi-input functions
(`COVAR_POP`/`COVAR_SAMP`, the window funnels) extend it directly and decide
for themselves which column a `null` disqualifies the row on, since that
follows from what each column is for — for a funnel the timestamp gates the
row, while a step predicate over `NULL` is already UNKNOWN and an extra field
is payload. That reasoning used to live on a private helper in each funnel
class and is now class-level Javadoc.
`NullableSingleInputAggregationFunction` is deleted.
**`RoaringBitmapUtils.foldUnset`** joins `forEachUnset`, and the `Reducer`
interface moves next to it. The two walks are written out separately on
purpose: routing either through the other would allocate an adapter lambda or a
mutable box on every call, and both sit on the aggregation path. The Javadoc on
each points at the other.
**Empty ranges** — the helpers already guaranteed that a consumer never
receives `from == to`, but 24 `if (to == from) { return; }` guards across 8
functions were defending against it anyway. Those are gone, and the invariant
is now stated where it is enforced rather than re-checked at each call site.
No functional change: no aggregation answer moves in either null-handling
mode. One behavioural mechanism did shift and is worth a reviewer's eye — the
zero-length block. `forEachNotNull` previously invoked `consume(0, 0)` when
null handling was off or the block had no null bitmap, so a function creating
its accumulator inside the callback would build an empty one; it now skips,
leaving the holder untouched. Every affected function either substitutes the
empty accumulator in `extractAggregationResult` or accepts `@Nullable` in
`extractFinalResult` and returns the same identity, and
`AggregationFunctionUtils.merge` is null-tolerant on both sides, so the result
is unchanged. The only caller that can pass a zero length is
`MultistageAggregationExecutor` for a filtered aggregation that matched no row,
and MSE always enables null handling; the single-stage path cannot, because
`DocIdSetOperator` returns `null` rather than a zero-doc block. That dependency
is now recorded at the guard
.
--
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]
