Skip to content

Prevent NaN centroid means in TDigestDouble (#702)#742

Open
PerumalsamyR wants to merge 1 commit into
apache:mainfrom
PerumalsamyR:fix-tdigest-nan-centroid-mean
Open

Prevent NaN centroid means in TDigestDouble (#702)#742
PerumalsamyR wants to merge 1 commit into
apache:mainfrom
PerumalsamyR:fix-tdigest-nan-centroid-mean

Conversation

@PerumalsamyR

Copy link
Copy Markdown

Addresses #702

Problem

When merging a value into an existing centroid, the mean was updated incrementally:

centroidMeans_[i] += ((values[current] - centroidMeans_[i]) * weights[current]) / centroidWeights_[i];

The intermediate (value - mean) (or its product with the weight) can overflow to infinity even when both operands are finite — e.g. means near opposite ends of the double range. Once a centroid mean becomes infinite, a subsequent update produces Inf + (-Inf) = NaN, which is then stored, breaking the sort/binary-search invariants and getting serialized. The same overflow pattern existed in weightedAverage(), used for quantile interpolation.

This mirrors the fix already applied to the Rust implementation in apache/datasketches-rust#23.

Changes

  • merge(): keep the incremental mean update in the common path (bit-identical results), but fall back to an overflow-safe weighted average m1*(w1/w) + m2*(w2/w) when the result is not finite. Each term of that form is bounded by the magnitude of its input, so the stored mean stays finite for finite inputs.
  • weightedAverage(): normalize the weights before multiplying so quantile interpolation between centroids of large magnitude cannot overflow.
  • update(): ignore ±Infinity in addition to NaN. With mixed infinities in the input, no update formula can keep centroid means well-defined, and rejecting them was the approach endorsed in the Rust PR discussion.
  • heapify() / compat formats: validate deserialized state as requested in the issue — min/max, centroid means and buffered values must be finite, centroid weights must be positive — throwing SketchesArgumentException otherwise, since serialized input cannot be assumed to be well-formed.

Testing

  • New test extremeValuesDoNotProduceNaN feeds 10k alternating ±Double.MAX_VALUE values: on current main it produces non-finite quantiles (and NaN centroid means); with this change all centroid means and quantiles stay finite.
  • New tests for update() ignoring NaN/±Inf and for heapify() rejecting corrupted bytes (NaN/Inf mean, NaN min, zero weight, NaN single value).
  • Full suite passes: mvn clean test — 2174 tests, 0 failures.

Merging centroids used the incremental mean update
mean + (value - mean) * w / weight, where the intermediate difference or
product can overflow to infinity for finite values of large magnitude,
eventually turning stored centroid means into NaN. Fall back to an
overflow-safe weighted average when that happens, and normalize weights
in weightedAverage so quantile interpolation cannot overflow either.

Also ignore infinite values in update(), consistent with NaN handling,
and validate deserialized state (finite min/max, centroid means and
buffered values, positive centroid weights), since serialized input
cannot be assumed to be well-formed.

Addresses apache#702
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant