Copilot commented on code in PR #19015:
URL: https://github.com/apache/pinot/pull/19015#discussion_r3626390845
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAccumulator.java:
##########
@@ -438,62 +544,113 @@ public double quantile(double quantile) {
}
double index = quantile * _totalWeight;
- if (index < _centroidWeights[0] / 2.0) {
- return _min + 2.0 * index / _centroidWeights[0] * (_centroidMeans[0] -
_min);
+ if (index < 1.0) {
+ return _min;
+ }
+ if (_centroidWeights[0] > 1.0 && index < _centroidWeights[0] / 2.0) {
+ return _min + (index - 1.0) / (_centroidWeights[0] / 2.0 - 1.0) *
(_centroidMeans[0] - _min);
}
- double weightSoFar = _centroidWeights[0] / 2.0;
int lastIndex = _numCentroids - 1;
+ if (index > _totalWeight - 1.0) {
+ return _max;
+ }
+ 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]);
+ }
+
+ double weightSoFar = _centroidWeights[0] / 2.0;
for (int i = 0; i < lastIndex; i++) {
double halfWeight = (_centroidWeights[i] + _centroidWeights[i + 1]) /
2.0;
if (weightSoFar + halfWeight > index) {
- double leftWeight = index - weightSoFar;
- double rightWeight = weightSoFar + halfWeight - index;
+ double leftUnitWeight = 0.0;
+ if (_centroidWeights[i] == 1.0) {
+ if (index - weightSoFar < 0.5) {
+ return _centroidMeans[i];
+ }
+ leftUnitWeight = 0.5;
+ }
+ double rightUnitWeight = 0.0;
+ if (_centroidWeights[i + 1] == 1.0) {
+ if (weightSoFar + halfWeight - index <= 0.5) {
+ return _centroidMeans[i + 1];
+ }
+ rightUnitWeight = 0.5;
+ }
+ double leftWeight = index - weightSoFar - leftUnitWeight;
+ double rightWeight = weightSoFar + halfWeight - index -
rightUnitWeight;
return weightedAverage(_centroidMeans[i], rightWeight,
_centroidMeans[i + 1], leftWeight);
}
weightSoFar += halfWeight;
}
- double leftWeight = index - _totalWeight - _centroidWeights[lastIndex] /
2.0;
- double rightWeight = _centroidWeights[lastIndex] / 2.0 - leftWeight;
- return weightedAverage(_centroidMeans[lastIndex], leftWeight, _max,
rightWeight);
+ double weightToMax = index - _totalWeight - _centroidWeights[lastIndex] /
2.0;
+ double weightFromLastCentroid = _centroidWeights[lastIndex] / 2.0 -
weightToMax;
+ return weightedAverage(_centroidMeans[lastIndex], weightToMax, _max,
weightFromLastCentroid);
}
private static double weightedAverage(double firstValue, double firstWeight,
double secondValue,
double secondWeight) {
if (firstValue > secondValue) {
return weightedAverage(secondValue, secondWeight, firstValue,
firstWeight);
}
+ if (firstWeight == 0.0) {
+ return secondValue;
+ }
+ if (secondWeight == 0.0 || firstValue == secondValue) {
+ return firstValue;
+ }
+ if (!Double.isFinite(firstValue)) {
+ return firstValue;
+ }
+ if (!Double.isFinite(secondValue)) {
+ return secondValue;
+ }
double average = (firstValue * firstWeight + secondValue * secondWeight) /
(firstWeight + secondWeight);
return Math.max(firstValue, Math.min(average, secondValue));
}
@Override
public Collection<Centroid> centroids() {
- return toTDigest().centroids();
+ compress();
+ List<Centroid> centroids = new ArrayList<>(_numCentroids);
+ for (int i = 0; i < _numCentroids; i++) {
+ centroids.add(Centroid.createWeighted(_centroidMeans[i],
Math.toIntExact((long) _centroidWeights[i]), null));
+ }
Review Comment:
`centroids()` currently forces each centroid weight through
`Math.toIntExact((long) ...)`, which will throw for valid digests whose
centroid weights exceed `Integer.MAX_VALUE` (Centroid counts are `long` in
t-digest 3.3, and Pinot explicitly tests large weights elsewhere). This makes
centroid iteration unexpectedly fragile for large-weight intermediate state.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAccumulator.java:
##########
@@ -429,7 +534,8 @@ public double quantile(double quantile) {
if (quantile < 0.0 || quantile > 1.0) {
throw new IllegalArgumentException("q should be in [0,1], got " +
quantile);
}
Review Comment:
`quantile(double)` does not reject `NaN` inputs. With the current check,
`quantile = Double.NaN` bypasses the range guard (all comparisons are false)
and then propagates `NaN` through the computation, which diverges from the
`TDigest` contract (and from `NonFiniteAwareTDigest`, which now rejects NaN).
--
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]
