xiangfu0 commented on code in PR #19795:
URL: https://github.com/apache/pinot/pull/19795#discussion_r4253840328
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/tdigest/TDigestAccumulator.java:
##########
@@ -536,59 +753,96 @@ public double quantile(double quantile) {
if (Double.isNaN(quantile) || quantile < 0.0 || quantile > 1.0) {
throw new IllegalArgumentException("q should be in [0,1], got " +
quantile);
}
+ if (_legacyDegraded) {
+ return Double.NaN;
+ }
materializePendingSerializedTDigest();
flush();
- if (_numCentroids == 0) {
+ return quantile(quantile, _numCentroids, _totalWeight, _min, _max,
_centroidMeans, _centroidWeights, null);
+ }
+
+ /// Interpolates the same distribution from primitive centroids or validated
encoded centroids, without a copy.
+ static double quantile(double quantile, int count, double totalWeight,
double min, double max, double[] means,
+ double[] weights, SerializedTDigestState encoded) {
+ if (count == 0) {
return Double.NaN;
}
- if (_numCentroids == 1) {
- return _centroidMeans[0];
+ if (count == 1) {
+ return mean(means, encoded, 0);
+ }
+ if (quantile == 0.0) {
+ return min;
+ }
+ if (quantile == 1.0) {
+ return max;
}
- double index = quantile * _totalWeight;
- if (index < 1.0) {
- return _min;
+ double index = quantile * totalWeight;
+ double firstWeight = weight(weights, encoded, 0);
+ if (firstWeight < 1.0 && index < firstWeight / 2.0) {
+ return weightedAverage(min, firstWeight / 2.0 - index, mean(means,
encoded, 0), index);
}
- if (_centroidWeights[0] > 1.0 && index < _centroidWeights[0] / 2.0) {
- return _min + (index - 1.0) / (_centroidWeights[0] / 2.0 - 1.0) *
(_centroidMeans[0] - _min);
+ if (firstWeight >= 1.0 && index < 1.0) {
+ return min;
}
- int lastIndex = _numCentroids - 1;
- if (index > _totalWeight - 1.0) {
- return _max;
+ if (weight(weights, encoded, 0) > 1.0 && index < weight(weights, encoded,
0) / 2.0) {
+ return min + (index - 1.0) / (weight(weights, encoded, 0) / 2.0 - 1.0) *
(mean(means, encoded, 0) - min);
}
- if (_centroidWeights[lastIndex] > 1.0
- && _totalWeight - index <= _centroidWeights[lastIndex] / 2.0) {
- return _max - (_totalWeight - index - 1.0) /
(_centroidWeights[lastIndex] / 2.0 - 1.0)
- * (_max - _centroidMeans[lastIndex]);
+ int lastIndex = count - 1;
+ double lastWeight = weight(weights, encoded, lastIndex);
+ if (lastWeight < 1.0 && index >= totalWeight - lastWeight / 2.0) {
+ return weightedAverage(mean(means, encoded, lastIndex), totalWeight -
index, max,
+ index - (totalWeight - lastWeight / 2.0));
+ }
+ if (lastWeight >= 1.0 && index > totalWeight - 1.0) {
+ return max;
+ }
+ if (weight(weights, encoded, lastIndex) > 1.0
+ && totalWeight - index <= weight(weights, encoded, lastIndex) / 2.0) {
+ return max - (totalWeight - index - 1.0) / (weight(weights, encoded,
lastIndex) / 2.0 - 1.0)
Review Comment:
**[high] `quantile()` returns NaN when the last live centroid has weight
exactly 2.0 and `index == totalWeight - 1`.**
The guard above uses strict `index > totalWeight - 1.0`, so the boundary
case `q = (W-1)/W` falls into this branch, where the divisor `weight/2.0 - 1.0`
is `0.0` and the numerator `totalWeight - index - 1.0` is also `0.0` — `0/0 =
NaN`.
Legacy t-digest 3.3 shares the raw formula, but this PR newly routes
payloads into the branch: zero-weight centroid filtering
(`SerializedTDigestInput.decode`) can make a weight-2 centroid the last live
one, and historical boundary repair can create one. Reproduced empirically:
verbose payload `means=[-40.17,-24.0,-21.23,-2.0], weights=[1,1,2,0]` →
`PERCENTILETDIGEST(..., 75)` returns NaN while t-digest 3.3 reading the
identical bytes returns -21.23. The NaN then flows into query results and
`SerializedTDigest.compareTo` ordering.
Suggested fix: mirror the lower tail (where `index < 1.0 → min` makes its
0/0 point unreachable), e.g. return `max` or interpolate within the last
centroid when `totalWeight - index <= 1.0`.
--
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]
