Copilot commented on code in PR #19201:
URL: https://github.com/apache/pinot/pull/19201#discussion_r3745982674
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/AggregationResultsBlock.java:
##########
@@ -124,73 +123,32 @@ public DataTable getDataTable()
return dataTableBuilder.build();
}
- boolean returnFinalResult = _queryContext.isServerReturnFinalResult();
- if (_queryContext.isNullHandlingEnabled()) {
- RoaringBitmap[] nullBitmaps = new RoaringBitmap[numColumns];
+ // NOTE: Nulls are serialized through the builder's null-aware path
regardless of the query's null-handling
+ // option. Aggregation functions whose accumulator has no identity element
(MINSTRING, MAXSTRING, ANYVALUE)
+ // return a null intermediate result in both modes, and their result
column type is not OBJECT.
+ dataTableBuilder.startRow();
+ if (_queryContext.isServerReturnFinalResult()) {
for (int i = 0; i < numColumns; i++) {
- nullBitmaps[i] = new RoaringBitmap();
- }
- dataTableBuilder.startRow();
- if (returnFinalResult) {
- for (int i = 0; i < numColumns; i++) {
- Object result =
_aggregationFunctions[i].extractFinalResult(_results.get(i));
- if (result == null) {
- result = columnDataTypes[i].getNullPlaceholder();
- nullBitmaps[i].add(0);
- }
- assert result != null;
+ Object result =
_aggregationFunctions[i].extractFinalResult(_results.get(i));
+ if (result == null) {
+ dataTableBuilder.setNull(i);
Review Comment:
The new `serverReturnFinalResult` null serialization path is not exercised
by the added regression tests; repository search finds only tests that reject
this option for merge-only reduction. Add a no-matching-rows aggregate test
with `serverReturnFinalResult=true` (in both null-handling modes) so the bitmap
write here and the unconditional read in `AggregationDataTableReducer` are
covered end to end.
##########
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:
The existing `MergeDataTablesOnlyTest.testNullHandlingAggregationRoundTrip`
enables null handling explicitly, so it does not cover this newly added
disabled-mode bitmap restoration (or the corresponding `setNull` output in
`buildIntermediateDataTable`). Add the same merge-only round trip without
`ENABLE_NULL_HANDLING` and assert that the null contribution remains null after
re-reduction.
--
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]
