yashmayya commented on code in PR #14181:
URL: https://github.com/apache/pinot/pull/14181#discussion_r1792822585
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/AggregationResultsBlock.java:
##########
@@ -116,7 +116,8 @@ public DataTable getDataTable()
}
dataTableBuilder.startRow();
for (int i = 0; i < numColumns; i++) {
- Object result = _results.get(i);
+ Object result =
+ returnFinalResult ?
_aggregationFunctions[i].extractFinalResult(_results.get(i)) : _results.get(i);
Review Comment:
> Does this mean we need to handle null in all extractFinalResult()
Shouldn't that already be the case for all the aggregation functions that
support null handling? I don't see such a check at other call-sites for
`AggregationFunction::extractFinalResult` and I checked multiple aggregation
function implementations (not all though), and it doesn't look like it should
be an issue anywhere? Ideally, from an aggregation function interface
standpoint too I think this is the right way to handle it - it seems a lot more
error prone to assume that some aggregation functions can't handle `null` in
`extractFinalResult` even though they can produce `null` as intermediate
result. While I couldn't find any such aggregation function, we should fix it
if any such implementations exist.
--
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]
