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: [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]
