Skip to content

Sort t-digest centroids by value, then by weight (#739) - #779

Open
luciakalf wants to merge 2 commits into
apache:mainfrom
luciakalf:fix-739-tdigest-sort
Open

luciakalf wants to merge 2 commits into
apache:mainfrom
luciakalf:fix-739-tdigest-sort

Conversation

@luciakalf

Copy link
Copy Markdown

Fixes #739

What

Sort.stableSort (used by TDigestDouble.merge) ordered centroids only by value. Centroids with equal values but different weights ended up in an order that depended on the input order and on the random pivot, so the same input could produce different merged sketches.

The sort now orders by value and, for equal values, by ascending weight, as agreed in the issue. It is pure Java with no new dependencies.

Changes

  • Sort.java: the quicksort and insertion sort compare (value, weight) pairs. The partition is a standard three-way partition around the pivot pair. Pairs equal to the pivot are identical, so the previous pivot-index handling is no longer needed.
  • SortTest.java: new tests for the example from the issue, independence from input order (shuffled input), agreement with a reference sort for sizes around the insertion-sort threshold (63/64/65), and sorting only the requested prefix.

Note: when reverseMerge_ is set, the sorted arrays are reversed, so equal values are then processed in descending weight. This is still deterministic.

Testing

  • mvn test on JDK 25: 2294 tests, 0 failures (including TDigestCrossLanguageTest and TDigestDoubleSerializationTest).
  • 3 of the 4 new tests fail on the previous implementation and pass with this change.
  • Building a TDigestDouble from the same 200,000 values with many duplicates 8 times produced 8 different serialized sketches before the change and 1 after. Quantiles are essentially unchanged.

Performance

Simple timing of Sort.stableSort (median, not JMH):

  • distinct values: no measurable difference (n=500: ~27 us)
  • many duplicates (20 distinct values): about 2x slower (n=10,000: 0.50 ms -> 1.05 ms), because equal values are now actually ordered by weight. At typical merge sizes this is a few microseconds.

This change was developed with the help of Claude (Anthropic), as suggested in the issue.

Equal values are now ordered by ascending weight, so the merge result no longer depends on the input order or on the random pivot choice.
@leerho

leerho commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

Thanks @luciakalf, this is a clean fix with no new dependencies, and the tests are good. It fixes a real bug: Sort.stableSort was never actually stable, because the random pivot left centroids with equal means in arbitrary order. As a result, identical input could produce different serialized sketches from run to run.

Before we merge, please make two small changes in TDigestDouble:

  1. Use StrictMath.log instead of Math.log in ScaleFunction.z(). The spec lets Math.log differ by up to 1 ulp between JVMs and platforms (it's usually an intrinsic). A 1-ulp difference can occasionally flip the proposedWeight <= ... test in merge(), which changes which centroids get combined. StrictMath.log is defined bit-for-bit (fdlibm), so it gives the same result everywhere.

  2. Compute normalizer once, before the while loop in merge(). Its inputs (k_ * 2 and centroidsWeight_) don't change inside the loop, so the result is bit-identical. It also means one log call per merge instead of one per centroid, which more than makes up for StrictMath.log being slower than the intrinsic.

Please also state the tie-break rule in the Javadoc: centroids are ordered by mean ascending, then weight ascending. Once released, this rule becomes part of the sketch's behavior.

What this achieves: with these changes, the same sequence of updates and merges should produce byte-identical sketches on any modern JVM (Java 17+), where all floating-point arithmetic is strict and Math.fma is exactly specified), on any platform. It does not make sketches byte-identical across languages (C++, Go, Rust, Python). Those implementations use a different formula to update a centroid's mean and different log implementations, so their results will still differ in the low-order bits. That's expected and isn't something this PR needs to solve.

Compatibility: the serialization format is unchanged, and images written before this change still deserialize and merge correctly, in Java and in the other languages.

I'll send a note to dev@ describing the tie-break rule so the other language implementations can adopt it if they choose.

@leerho

leerho commented Oct 24, 2026

Copy link
Copy Markdown
Member

Background on the tie-break rule, its effect on other language implementations, and why byte-identical images across languages aren't achievable for t-digest: #780

@luciakalf

Copy link
Copy Markdown
Author

Thanks for the review! I’ve pushed the changes: ScaleFunction.z() now uses StrictMath.log, the normalizer is computed once before the loop in merge(), and the class Javadoc states the tie-break rule (mean ascending, then weight ascending). Sort.stableSort documents the same rule. mvn test passes on JDK 25.

This branch has not been deployed

No deployments
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.

can we use stable sort for tdigest

2 participants