Jackie-Jiang commented on code in PR #19201:
URL: https://github.com/apache/pinot/pull/19201#discussion_r3751889134
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/AggregationDataTableReducer.java:
##########
@@ -102,14 +102,11 @@ private Object[] mergeIntermediateResults(DataSchema
dataSchema, Collection<Data
AggregationFunction aggregationFunction = _aggregationFunctions[i];
Object intermediateResultToMerge;
ColumnDataType columnDataType = dataSchema.getColumnDataType(i);
- if (_queryContext.isNullHandlingEnabled()) {
- RoaringBitmap nullBitmap = dataTable.getNullRowIds(i);
- if (nullBitmap != null && nullBitmap.contains(0)) {
- intermediateResultToMerge = null;
- } else {
- intermediateResultToMerge =
-
AggregationFunctionUtils.getIntermediateResult(aggregationFunction, dataTable,
columnDataType, 0, i);
- }
+ // Nulls are restored regardless of the query's null-handling option:
an aggregation function whose
+ // accumulator has no identity element returns a null intermediate
result in both modes.
+ RoaringBitmap nullBitmap = dataTable.getNullRowIds(i);
+ if (nullBitmap != null && nullBitmap.contains(0)) {
+ intermediateResultToMerge = null;
Review Comment:
Confirmed — `testNullHandlingAggregationRoundTrip` sets
`ENABLE_NULL_HANDLING=true`, so the disabled-mode restore here and the matching
`setNull` in `buildIntermediateDataTable` were both uncovered. Added coverage.
One correction to the suggestion, though: reusing that test's `MIN(m)`
DOUBLE fixture without the option would have been a bad test. `MIN`'s
accumulator is primitive and always holds an identity value, so no server can
emit a null DOUBLE with null handling disabled — the fixture would be an input
production never produces. It would also have failed for an unrelated reason,
since `MinAggregationFunction#merge` only null-guards under `if
(_nullHandlingEnabled)` and unboxes directly otherwise.
So the new tests use `MAXSTRING`, whose accumulator genuinely has no
identity element (hence null in either mode) and whose `merge` is
`@Nullable`-safe:
- `testNullAggregationRoundTripWithNullHandlingDisabled` — one server
contributes a value, one contributes null.
- `testAllNullAggregationRoundTripWithNullHandlingDisabled` — every server
contributes null, so the merged intermediate itself carries the null and has to
survive `buildIntermediateDataTable`'s write as well as the restore.
--
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]
