xiangfu0 commented on code in PR #19795:
URL: https://github.com/apache/pinot/pull/19795#discussion_r4222565486
##########
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:
**[reuse] `mergeMeans` and `TDigestUtils.weightedMean` are near-verbatim
duplicates of the same numeric kernel**, and this PR patched both with the
identical sign-aware lerp + clamp fix. The copies already differ subtly (one
handles infinite means, the other relies on callers), so the next accuracy fix
landing in only one of them yields different centroid means for
merge-at-serialize (`reduceVerboseCentroids`) vs merge-at-accumulate. Share one
helper.
--
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]
