xiangfu0 commented on code in PR #19795:
URL: https://github.com/apache/pinot/pull/19795#discussion_r4223013557
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/PercentileTDigestAccumulator.java:
##########
@@ -1325,10 +1347,10 @@ private static double mergeMeans(double firstMean,
double firstWeight, double se
if (firstMean == secondMean) {
return firstMean;
}
- if (sameSignMeans || Math.copySign(1.0, firstMean) == Math.copySign(1.0,
secondMean)) {
- return firstMean + (secondMean - firstMean) * secondWeight / totalWeight;
- }
- return firstMean * (firstWeight / totalWeight) + secondMean *
(secondWeight / totalWeight);
+ double average = sameSignMeans || Math.copySign(1.0, firstMean) ==
Math.copySign(1.0, secondMean)
+ ? firstMean + (secondMean - firstMean) * (secondWeight / totalWeight)
Review Comment:
Accumulator merges and compatibility reduction now use one bounded
weighted-mean kernel in TDigestUtils. The native caller retains its established
same-sign fast path; the reducer passes the normal sign-check path. The
duplicate arithmetic implementation is removed, with the unchanged accuracy
envelope and numerical regressions retained.
Addressed in
[33c0b75b60](https://github.com/apache/pinot/commit/33c0b75b60f5ed7561cbdc186e241167992c0a2e).
All 491 selected tests, the 322-case compatibility matrix and 74 real-reader
cases pass.
--
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]
