Jackie-Jiang commented on PR #19211: URL: https://github.com/apache/pinot/pull/19211#issuecomment-5250122723
@yashmayya thanks — the nit is folded in, but I think the NPE is against pre-#19158 code. ## The merge path The snippet you quoted is what `AggregationDataTableReducer` looked like before #19158. That PR replaced it: ```diff - intermediateResults[i] = aggregationFunction.merge(mergedIntermediateResult, intermediateResultToMerge); + AggregationFunctionUtils.merge(aggregationFunction, intermediateResults[i], intermediateResultToMerge); ``` `AggregationFunctionUtils.merge` settles **both** operands before delegating, so `merge` is never handed a null from there. `IndexedTable.updateRecord`, the group-by path you mention, routes through the same helper. The supporting evidence points the same way: `VarianceAggregationFunction.merge` on master today is a bare `intermediateResult1.apply(intermediateResult2)` with no guards on either side. #19158 removed them precisely because the caller now settles the identity, and the contract says `merge` is handed two real values and must not be called with `null`. So I have not added the guard or the `@Nullable` parameters — they would contradict that contract and be unreachable. If you can point at a caller that still reaches `merge` with a null, I will fix it there rather than in the implementation, since that would be a bug in the caller. Your read of MSE matches mine: both executors skip a null intermediate before merging. ## The nit Folded in. `COVAR_SAMP` over a single contributing row divides by `count - 1`, so it is undefined; it now answers `NULL` with the option on, where it returned `-Infinity` in both modes before. ## Also changed since your review From the Copilot comments: the two-block null merge now reuses `NullableSingleInputAggregationFunction.orNullIterator` rather than materializing a union with `RoaringBitmap.or` — that helper already existed and I had missed it — and `NullHandlingEnabledQueriesTest` adds end-to-end coverage over segments with nulls. The description also now states the mixed-version behaviour in the disabled mode, which it previously described as unaffected. -- 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]
