yashmayya commented on PR #19211:
URL: https://github.com/apache/pinot/pull/19211#issuecomment-5247336757
`extractAggregationResult` can now return `null`, but `merge` still calls
`intermediateResult1.apply(intermediateResult2)`, which dereferences its
argument right away. A null second argument throws an NPE.
The SSE reduce path can pass one.
`AggregationDataTableReducer.mergeIntermediateResults` guards the accumulator,
not the incoming value:
```java
if (mergedIntermediateResult == null) {
intermediateResults[i] = intermediateResultToMerge;
} else {
intermediateResults[i] =
aggregationFunction.merge(mergedIntermediateResult, intermediateResultToMerge);
}
```
A few lines above, it sets `intermediateResultToMerge` to `null` when the
null bitmap of the data table marks row 0.
So `SELECT COVAR_POP(x, y) FROM t WHERE ...` with `enableNullHandling=true`
breaks when one server matches no rows and another returns data. If the data
table with data comes first, the next merge throws. `IndexedTable.updateRecord`
has the same shape on the group-by path.
`VarianceAggregationFunction.merge` already guards both sides when
`_nullHandlingEnabled` is true. Covariance needs the same guard, plus
`@Nullable` on both parameters.
MSE is safe here. `MultistageAggregationExecutor` and
`MultistageGroupByExecutor` both skip a null intermediate result before they
call merge.
Nit, and pre-existing: with null handling on, `COVAR_SAMP` over a single
non-null row still returns `-Infinity` from the `count - 1 == 0` branch. It
sits close to what this PR corrects, so it can be worth folding in.
The rest reads well to me. The union of the two null bitmaps is correct and
does not mutate either block's bitmap, the disabled path keeps the old
behavior, and the tests cover the interesting cases.
--
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]
