You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2026/09/23 21:21:00 UTC
[PR] Move polymorphic aggregate construction into per-family providers [pinot]
xiangfu0 opened a new pull request, #19646:
URL: https://github.com/apache/pinot/pull/19646
## Summary
`AggregationFunctionFactory` builds every aggregate in one large switch. For a few families, that switch also contains the family's own argument validation and type dispatch: for example, it maps the `'dataType'` literal of `FIRST_WITH_TIME`, `LAST_WITH_TIME` and `ARRAY_AGG` to an implementation class.
This PR moves those families behind a small `AggregationFunctionProvider` interface. Each provider receives the whole `FunctionContext` and owns its family's construction logic:
| Aggregate | Provider |
| --- | --- |
| `MODE` | `ModeAggregationFunction.Provider` |
| `ANY_VALUE` | `AnyValueAggregationFunction.Provider` |
| `FIRST_WITH_TIME` / `LAST_WITH_TIME` | `FirstLastWithTimeAggregationFunctionProvider.First` / `.Last` |
| `ARRAY_AGG` | `ArrayAggFunctionProvider` |
| ExprMin/ExprMax parent and child | `ParentExprMinMaxAggregationFunction.MinProvider` / `.MaxProvider`, `ChildExprMinMaxAggregationFunction.MinProvider` / `.MaxProvider` |
**No behavior change.** The providers construct the same classes with the same arguments and the same validation messages. The factory consults the registry inside its existing `try` block, so errors are still wrapped in the same `BadQueryRequestException`. Aggregates without a provider keep using the switch.
## Why
These are the aggregates whose result type depends on their input. Follow-up work (#19523) binds them to schema-derived logical types, so each family needs the whole call rather than one factory case per type. Keeping the dispatch with the family means later changes touch the family, not the core factory.
## Registry
- `AggregationFunctionProviderRegistry` loads providers once with `ServiceLoader` into an immutable `EnumMap`. It is safe for concurrent query construction.
- A second provider for the same aggregate fails at initialization instead of depending on classpath order.
- The providers are registered in `pinot-core`'s `META-INF/services`. Shaded builds already merge service files: the root `pom.xml` configures `ServicesResourceTransformer`.
## Tests
- New `AggregationFunctionProviderRegistryTest`: every moved family is discoverable, duplicates are rejected, and the registry is immutable.
- Existing tests still pass unchanged, covering construction for every type option, execution, and null handling:
- `AggregationFunctionFactoryTest`
- the MODE, ANY_VALUE, FIRST/LAST_WITH_TIME, ARRAY_AGG and ExprMin/Max function and query tests
- `AggregationFunctionNullContractTest`
Split out of #19523.
--
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] Move polymorphic aggregate construction into per-family providers [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19646:
URL: https://github.com/apache/pinot/pull/19646#issuecomment-5803263242
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19646?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 0.00%. Comparing base ([`33d88e4`](https://app.codecov.io/gh/apache/pinot/commit/33d88e480ba7e05528e619c34e100b0dc3965db6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`fa474cd`](https://app.codecov.io/gh/apache/pinot/commit/fa474cd7214f56de6614b442bed1ba0c72eea945?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (33d88e4) and HEAD (fa474cd). Click for more details.
>
> <details><summary>HEAD has 18 uploads less than BASE</summary>
>
>| Flag | BASE (33d88e4) | HEAD (fa474cd) |
>|------|------|------|
>|lane-b|2|1|
>|integration|3|1|
>|java-25|5|1|
>|temurin|5|1|
>|lane-a|1|0|
>|integration1|2|0|
>|unittests1|1|0|
>|unittests|2|0|
>|unittests2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19646 +/- ##
=============================================
- Coverage 67.93% 0.00% -67.94%
=============================================
Files 3511 3 -3508
Lines 227353 6 -227347
Branches 35966 0 -35966
=============================================
- Hits 154451 0 -154451
+ Misses 60762 6 -60756
+ Partials 12140 0 -12140
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19646/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/19646/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19646/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19646/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19646/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-67.94%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19646/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19646/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/19646/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-67.94%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19646/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/19646/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19646/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/19646?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
Re: [PR] Move polymorphic aggregate construction into per-family providers [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #19646:
URL: https://github.com/apache/pinot/pull/19646
--
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