xiangfu0 commented on code in PR #19795:
URL: https://github.com/apache/pinot/pull/19795#discussion_r4229464040
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/PercentileTDigestValueAggregator.java:
##########
@@ -284,52 +270,113 @@ public void add(double value) {
}
@Override
- public void add(double value, int weight) {
+ public void add(double value, double weight) {
if (Double.isNaN(value)) {
throw new IllegalArgumentException("Cannot add NaN to t-digest");
}
- if (weight <= 0) {
+ if (!(weight > 0.0) || !Double.isFinite(weight)) {
throw new IllegalArgumentException("TDigest weight must be positive: "
+ weight);
}
- invalidateCaches();
+ requireMutable();
+ checkTotalWeight(getTotalWeight() + weight);
if (value == Double.NEGATIVE_INFINITY) {
- _negativeInfinityWeight = Math.addExact(_negativeInfinityWeight,
weight);
+ _negativeInfinityWeight += weight;
} else if (value == Double.POSITIVE_INFINITY) {
- _positiveInfinityWeight = Math.addExact(_positiveInfinityWeight,
weight);
+ _positiveInfinityWeight += weight;
} else {
_finiteDigest.add(value, weight);
}
+ mutated();
}
@Override
public void add(TDigest other) {
+ if (other.isEmpty()) {
+ return;
+ }
+ requireMutable();
+ if (!other.hasValidStatistics() && !isEmpty()) {
+ throw corruptedMutation();
+ }
+ if (other.hasValidStatistics()) {
+ checkTotalWeight(getTotalWeight() + other.getTotalWeight());
+ }
if (other instanceof NonFiniteAwareTDigest) {
NonFiniteAwareTDigest wrapped = (NonFiniteAwareTDigest) other;
- long negativeInfinityWeight = wrapped._negativeInfinityWeight;
- long positiveInfinityWeight = wrapped._positiveInfinityWeight;
- // MergingDigest.add(List) copies the implementation's
double-precision weight arrays directly. The generic
- // add(TDigest) path goes through Centroid.count(), which is limited
to an int and truncates large weights.
- wrapped.invalidateCaches();
- invalidateCaches();
- if (wrapped._finiteDigest.size() > 0L) {
- _finiteDigest.add(List.of(wrapped._finiteDigest));
+ double negativeInfinityWeight = wrapped._negativeInfinityWeight;
+ double positiveInfinityWeight = wrapped._positiveInfinityWeight;
+ if (!wrapped._finiteDigest.isEmpty()) {
Review Comment:
**Bug: wrapper merge drops fractional-infinity boundary provenance when the
source's finite digest is empty, later aborting the segment/star-tree build.**
Historical fractional-boundary provenance
(`_historicalFractionalMinMean/MaxMean`) lives on the finite accumulator and
transfers between `NonFiniteAwareTDigest` wrappers only through
`_finiteDigest.add(wrapped._finiteDigest)` inside this `if
(!wrapped._finiteDigest.isEmpty())` guard. A historical payload whose
positive-weight mass is entirely infinite with a fractional boundary weight
(e.g. one `(+Inf, 0.5)` centroid, `min=max=+Inf`) produces a wrapper whose
finite digest is empty but carries the provenance recorded by `fromBytes` via
`recordHistoricalFractionalBoundaries`. Merging it transfers the 0.5 infinity
weight but not the provenance, so serializing the target later fails
`hasInheritedFractionalBoundaryEncoding` (provenance is NaN) and
`checkLegacyBoundaryWeights` throws `Cannot serialize fractional TDigest
boundary mass for legacy readers`, aborting the whole build on data that was
previously serializable. (Reversed merge order survives via `addAccumulator`'s
provenance in
heritance.) The infinity weights and the provenance need to be transferred
even when the finite digest is empty.
--
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]
