Jackie-Jiang opened a new pull request, #19158: URL: https://github.com/apache/pinot/pull/19158
## Summary Null handling is a per-query flag, and the two modes place very different requirements on an `AggregationFunction` — but that contract was never written down. Implementations drifted apart as a result: some resolved the empty multiset, some threw on it, some carried defensive null branches no caller could reach, and `@Nullable` annotations disagreed with the code they annotated. This writes the contract onto `AggregationFunction` and brings the implementations in line with it. ### The contract - **Null handling disabled** — nulls read as the column default, the accumulator is primitive, and no null tracking happens. An untouched accumulator is indistinguishable from one that aggregated to the type's identity, so the empty multiset has no representation: its answer is whatever the initial accumulator renders to (`0` for `SUM`, `+Infinity` for `MIN`). This is a performance path, and those answers are a backward-compatibility constraint rather than an attempt at SQL conformance. - **Null handling enabled** — SQL semantics. A `null` intermediate result means the empty multiset, and `extractFinalResult` is the only place that decides what that means for a given function: `0` for the counting functions, `null` for the value functions. One consequence worth calling out: object-backed accumulators whose type has no identity to render (`MAXSTRING`, `MINSTRING`, `ANYVALUE`) return `null` even with the flag off, so `extractFinalResult` has to accept `null` in both modes. ### Fixes an NPE in the broker response path `PERCENTILERAWEST`, `PERCENTILERAWKLL`, `PERCENTILERAWTDIGEST` and their MV variants wrapped the intermediate result in a serializer (`SerializedQuantileDigest` / `SerializedKLL` / `SerializedTDigest`) without checking it. Those wrappers dereference what they are handed in both `toString()` and `compareTo()`, so a `null` intermediate did not fail at extraction — it failed later, when the broker rendered the value. For the KLL variants this is reachable on the single-stage engine in **both** modes, because `PercentileKLLAggregationFunction.extractAggregationResult` returns the holder's value directly and that is `null` for an untouched holder. A query whose segments are all pruned hits it. ### Settles the merge identity once, in the caller The empty multiset is the identity of merging and means the same thing for every aggregation, so it does not belong in each implementation. `AggregationFunctionUtils#merge` and `#mergeFinalResult` resolve a `null` operand and only then delegate, and all six call sites route through them. Two of those call sites — `AggregationResultsBlockMerger` and `SortedRecordsMerger` — previously had no null handling at all; two others carried `// TODO: Fix it` blocks that this removes. With the identity settled in one place, `merge` implementations only ever see two real values, so the null branches inside them are unreachable and have been dropped. ### Also in this change - `extractFinalResult` resolves the empty multiset across the remaining functions instead of dereferencing it. - `@Nullable` annotations are aligned with what each implementation actually does — added where a method genuinely returns `null`, removed from `merge` parameters where the annotation contradicted the contract. - `BaseBooleanAggregationFunction.aggregateGroupByMV` mirrors its single-value counterpart and skips null rows when null handling is enabled. - The star-tree pre-aggregated branches of `COUNT`, `AVG`, `COUNTMV` and `SUMMV` are made null-aware. These are inert against today's star-tree, which emits no null vector; they are groundwork for null-aware star-tree support. ### Tests `AggregationFunctionNullContractTest` enforces the contract against every aggregation function that can be constructed generically — 95 of the 103 `AggregationFunctionType` values, under both flag settings. The eight it cannot construct are pinned in both directions, so a newly added function cannot drop out of the contract unnoticed and a stale exclusion cannot linger. It asserts that the final result *renders*, not merely that extraction returned. That distinction is what the raw percentile bug turned on: extraction succeeded and the failure surfaced downstream. `BooleanAggQueriesTest` gains multi-value group-by coverage for `BOOL_AND` / `BOOL_OR`, including groups that separate skipping a null from folding it in as the column default. ### Known deviations Two are recorded as a `TODO` on the interface rather than addressed here: - Several aggregation methods still fold the column default into the aggregate instead of skipping null rows when null handling is enabled: the distinct-count family, the tuple and frequency sketches, the statistical functions, the first/last-with-time functions, and the funnel family. Follow-up change. - The multi-stage engine constructs every aggregation function with null handling enabled and never consults the query option, so a query that disables it still gets enabled-mode semantics there. `SUM` over an all-pruned query is `NULL` on the multi-stage engine and `0` on the single-stage engine. This may well be intended, given the multi-stage engine is the SQL-conformant one — flagging it rather than changing it. -- 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]
