gortiz commented on code in PR #15525:
URL: https://github.com/apache/pinot/pull/15525#discussion_r2044442718
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/MinMaxRangeAggregationFunction.java:
##########
@@ -81,10 +84,13 @@ public void aggregate(int length, AggregationResultHolder
aggregationResultHolde
for (int i = from; i < to; i++) {
MinMaxRangePair minMaxRangePair =
ObjectSerDeUtils.MIN_MAX_RANGE_PAIR_SER_DE.deserialize(bytesValues[i]);
minMax.apply(minMaxRangePair);
+ empty.set(false);
}
});
}
- setAggregationResult(aggregationResultHolder, minMax.getMin(),
minMax.getMax());
+ if (!empty.get()) {
Review Comment:
The code may be clearer, but I don't think the suggested version will be
faster than the current code. It probably is depreciable, but I would say this
code could be even faster than the one in #15545. I'm fine with both versions,
and I would even say I prefer the new one because it is easier to read, but I
don't see how that can affect performance.
The difference between reading an atomic boolean modified by a single thread
and comparing two primitives (through a reference) should always favor the
atomic boolean. But even if it isn't, the cost of all other instructions in
this method are going to hide any tiny difference between one and the other
version
--
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]
