You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2026/08/24 23:57:35 UTC
[PR] Extract null-skipping helpers into NullSkippingUtils and BaseAggregationFunction [pinot]
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Extract null-skipping helpers into NullSkippingUtils and BaseAggregationFunction; tidy aggregation tests [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #19355:
URL: https://github.com/apache/pinot/pull/19355
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Extract null-skipping helpers into NullSkippingUtils and BaseAggregationFunction; tidy aggregation tests [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19355:
URL: https://github.com/apache/pinot/pull/19355#issuecomment-5403799300
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19355?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 100.00%. Comparing base ([`4f7023a`](https://app.codecov.io/gh/apache/pinot/commit/4f7023a564ab8363645497a4df7297f54f472283?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e67637e`](https://app.codecov.io/gh/apache/pinot/commit/e67637e8b5b4bbab7a7e483dfac851d35cbe2646?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19355 +/- ##
==============================================
+ Coverage 57.87% 100.00% +42.12%
+ Complexity 7 6 -1
==============================================
Files 2668 3 -2665
Lines 162330 6 -162324
Branches 26272 0 -26272
==============================================
- Hits 93948 6 -93942
+ Misses 60410 0 -60410
+ Partials 7972 0 -7972
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19355/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19355/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19355/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19355/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19355/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+42.12%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19355/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19355/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19355/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+42.12%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19355/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19355/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19355?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org