Jackie-Jiang opened a new pull request, #19211: URL: https://github.com/apache/pinot/pull/19211
## Summary `COVAR_POP` and `COVAR_SAMP` never received the query's null handling option. Their constructor did not take it, so they folded the column's default value into the aggregate whatever the query asked for, and `extractFinalResult` could not tell the two modes apart when rendering an empty input. This is the first of the follow-ups recorded as known deviation 1 on `AggregationFunction` in #19158. ### Behavior change With `enableNullHandling=true`, these functions now skip null rows and answer `NULL` over an input where nothing was aggregated, where they previously folded in the default and answered `-Infinity`. **On the multi-stage engine the change is unconditional, and no query option controls it.** `AggregateOperator` builds every aggregation function with null handling enabled and never consults the query's option, so these functions previously discarded a flag the engine had already set to `true`. Worth a release note on the same footing as the multi-value functions in #19158. With the option disabled, nothing changes: the column default is still folded in, and an untouched accumulator still renders `-Infinity`. ### A row counts only when both columns are present A covariance pairs one value from each of two columns, so a row contributes only if neither value is null. The rows skipped are therefore those in the **union** of the two columns' null bitmaps, not either one alone. There is no multi-input nullable base to inherit this from — `forEachNotNull` is a single-block instance method on `NullableSingleInputAggregationFunction` — so this adds a private two-block variant that walks the ranges unset in the union. It builds the union with `RoaringBitmap.or`, so neither block's own bitmap is mutated. If the funnel and tuple-sketch groups need the same helper when their turn comes, it should move somewhere shared rather than be copied. ### Conforms the substitution at the same time `extractAggregationResult` returned a zero-count `CovarianceTuple` for an untouched holder, which destroyed the "nothing was aggregated" signal before `extractFinalResult` could act on it. Both extraction methods now return `null`, and `extractFinalResult` renders the disabled-mode identity. The contract notes that this is not separable from the option: without it, `extractFinalResult` has nothing to decide with, so the substitution was the only thing holding the answer. Deviation 1 is updated to drop the covariance functions. ### Tests `CovarianceAggregationFunctionTest` covers the both-columns rule, an all-null input answering `NULL`, the disabled mode still folding the default, and the identity being rendered only when the option is off. The union is the novel part, so it is mutation-tested: weakening it to the first column's bitmap makes `testRowIsSkippedWhenEitherColumnIsNull` fail with `expected [2] but found [3]`. `AggregationFunctionNullContractTest` detected the change on its own — `COVARPOP` and `COVARSAMP` appeared as newly honouring the option — and they are pinned in `HONOURS_NULL_HANDLING` accordingly. -- 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]
