Prevent NaN centroid means in TDigestDouble (#702)#742
Open
PerumalsamyR wants to merge 1 commit into
Open
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #702
Problem
When merging a value into an existing centroid, the mean was updated incrementally:
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 producesInf + (-Inf) = NaN, which is then stored, breaking the sort/binary-search invariants and getting serialized. The same overflow pattern existed inweightedAverage(), 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 averagem1*(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 — throwingSketchesArgumentExceptionotherwise, since serialized input cannot be assumed to be well-formed.Testing
extremeValuesDoNotProduceNaNfeeds 10k alternating±Double.MAX_VALUEvalues: on current main it produces non-finite quantiles (and NaN centroid means); with this change all centroid means and quantiles stay finite.update()ignoring NaN/±Inf and forheapify()rejecting corrupted bytes (NaN/Inf mean, NaN min, zero weight, NaN single value).mvn clean test— 2174 tests, 0 failures.