Jackie-Jiang opened a new pull request, #19326:
URL: https://github.com/apache/pinot/pull/19326
## Summary
Part of #19218. Follows #19316, which did the sketch-backed distinct counts.
`SKEWNESS`, `KURTOSIS`, `STUNION`, `SUMARRAYLONG`, `SUMARRAYDOUBLE`,
`HISTOGRAM` and `IDSET` never received
the query's null handling option, so a null row was aggregated as the column
default whatever the query
asked for. They now extend `NullableSingleInputAggregationFunction` and skip
null rows when the option is
enabled, across `aggregate`, `aggregateGroupBySV` and `aggregateGroupByMV`.
Five of the seven were not even
being passed the flag by `AggregationFunctionFactory`.
### The empty answer is per-function, and each one is a constraint
These seven disagree on what "nothing aggregated" renders to with the option
disabled: `NaN` for the moments,
the empty point for `STUNION`, an all-zero histogram, an empty id set, and
`NULL` for the array sums. Each is
the answer that mode has always given, so the substitution of an empty
accumulator for an untouched holder is
kept and guarded on the flag rather than removed. The disabled path —
including the intermediate result that
crosses the wire — is unchanged; only the enabled path yields the `null`
that means nothing was aggregated.
`SUMARRAYLONG` / `SUMARRAYDOUBLE` need no such guard, because `NULL` is
already their answer in both modes.
That is now pinned by a test rather than left to inspection.
### Multi-value support
- **`HISTOGRAM`** carried a `//TODO: Add MV support for histogram`, rejected
multi-value columns in two of its
three methods, and failed with a different error in the third. It now
supports them, and also `BIG_DECIMAL`,
which it previously rejected as a *"non-numeric type"*.
- **`STUNION`** read the single-value representation in all three paths. A
multi-value row now folds every
geometry into the same union.
- **`IDSET`** already handled both, but was missing the multi-value `BYTES`
case even though `IdSets` supports
`BYTES` and the single-value case worked. SV and MV are now symmetric
across all six types it supports.
All three are restructured into three entry points dispatching to private
SV/MV helpers, matching the
sketch-backed distinct counts.
### A bug the restructure exposed
`forEachNotNull` invokes its consumer once per *contiguous non-null range*.
`HISTOGRAM` accumulates into a
local buffer and publishes it, so publishing from inside the lambda re-added
the earlier ranges' counts on
every later range — for rows `[v0, null, v2]`, `v0` was counted twice. It is
now accumulated with
`foldNotNull` and published once, guarded on a row count so an all-null
block still leaves the holder
untouched. The other six accumulate directly into the holder and were never
affected.
### What is deliberately left out
`SKEWNESS` and `KURTOSIS` keep their single-value, non-`BIG_DECIMAL`
restriction. Both limits live in the
shared `StatisticalAggregationFunctionUtils.getValSet`, which `VARPOP`,
`VARSAMP`, `STDDEVPOP`, `STDDEVSAMP`
and both covariances also route through, and which carries its own MV TODO.
Widening it silently changes six
other functions, so it belongs in its own change.
### Testing
`ValueAggregationNullHandlingTest` is new. It covers what
`AggregationFunctionNullContractTest` cannot: that
harness drives one synthetic single-value block through `aggregate` only,
and cannot construct most of these
from its shared argument shapes at all.
Every case was verified by mutation rather than by inspection. One mutation
survived the first pass —
removing the empty-range guard from the array sums — which is what the
zero-length block test now pins.
`AggregationFunctionNullContractTest` gains `SKEWNESS`, `KURTOSIS`,
`STUNION`, `HISTOGRAM`, `IDSET` and both
array sums in the set that must answer `NULL` when nothing was aggregated,
and pins `SKEWNESS`/`KURTOSIS`/
`IDSET`/`HISTOGRAM` as newly honouring the option. `SyntheticBlockValSets`
gains the `Int`, `Float`, `Double`
and `BigDecimal` multi-value fixtures the new paths need.
--
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]
