Fix O(n²) compress on duplicate/low-cardinality input (MOD-17228)

[fcostaoliveira]

Problem

td_qsort (used by every td_compress) uses a single central pivot and recurses into both partitions. On a run of equal keys the partition places nothing on the "less" side, splitting off one element per level:

• O(n²) comparisons, and
• O(n) recursion depth.

Duplicate / low-cardinality input is t-digest's common case — it exists to summarize streams of repeated measurements (latencies, counters), not an adversarial one. So a td_compress over N buffered equal values runs in quadratic time.

Because td_compress runs inline on the calling thread, a large such compress stalls that thread for the full O(n²). Measured through the RedisBloom TDIGEST commands on Redis 8.8.1 (internal ref MOD-17228): 430k identical values → one TDIGEST.QUANTILE (which triggers compress) blocks the server ~98 s (a bystander PING on a second connection goes from ~2 ms to ~98 s).

Fix

Replace the sort with 3-way (Dutch-national-flag) partitioning + tail-recursion elimination:

• equal keys collapse in a single pass → O(n log n) even for all-equal input;
• recursing only into the smaller partition bounds the stack to O(log n).

The sort is guaranteed O(n log n) (introsort with heapsort fallback). Results stay within t-digest's accuracy guarantee, but are NOT centroid-for-centroid identical for weighted duplicates (equal-key order differs) — see the review thread.

Single compress of N identical values, before → after (same machine):

N	before	after	speedup
50,000	1,238 ms	1.0 ms	~1,200×
100,000	4,994 ms	2.0 ms	~2,500×
200,000	20,069 ms	3.2 ms	~6,300×
5,000,000	~hours	80 ms	—

Testing

• Existing unit suite passes unchanged.
• New test test_duplicate_heavy_compress (all-equal / ascending / descending, 50k each) asserting sorted centroids and correct quantiles for the inputs that were pathological for the old sort.
• 20,000 randomized parallel-array sort trials (few-distinct-value) — sorted + weights permuted in lock-step.
• ASan / UBSan clean.

🤖 Generated with Claude Code

---

Note

Medium Risk
Core compress/sort path and td_add API behavior change (±Inf/NaN now rejected); performance and digest details on duplicate-heavy streams change, though bounded by design and heavily tested.

Overview
Fixes MOD-17228 by replacing the central-pivot quicksort used in td_compress with introsort: 3-way partitioning for duplicate-heavy centroids, median-of-three pivots, insertion sort on small ranges, recursion only on the smaller partition, and heapsort when the depth limit is hit so worst-case time is O(n log n) with O(log n) stack instead of quadratic work and deep recursion on all-equal input.

td_add now returns EINVAL for non-finite means (NaN and ±Inf), documented in tdigest.h, so sort order and centroid merge math stay well-defined.

Tests add td_sort_complexity_test (optional TD_INSTRUMENT_SORT comparison/fallback counters), unit coverage for non-finite adds, large duplicate-heavy compress correctness, and weighted-duplicate quantile accuracy. Equal-key tie order can differ from the old sorter; accuracy remains within t-digest guarantees.

^{Reviewed by Cursor Bugbot for commit 1ced90f. Bugbot is set up for automated code reviews on this repo. Configure here.}

[gabsow]

I found one blocking issue and two coverage/compatibility gaps.

[P1] The deterministic midpoint pivot still permits O(n²) input.

Three-way partitioning fixes the all-equal/low-cardinality trigger, and tail-recursion elimination bounds stack depth, but the sort still selects means[lo + (hi - lo) / 2] deterministically. A client can provide a static permutation of distinct values that makes each selected pivot the smallest remaining value, so every partition removes only one element and the runtime remains quadratic.

I reproduced this against this PR's compiled head:

Values	td_compress CPU time
10,000	0.062 s
20,000	0.207 s
40,000	0.807 s
80,000	3.087 s

The near-4× increase on doubling confirms quadratic scaling; the same curve projects to roughly 89 seconds at 430k values on this machine. This narrows MOD-17228's trigger but does not eliminate the remotely-triggerable stall. Please use a guaranteed O(n log n) strategy, such as introsort with a heapsort fallback, while continuing to permute means and weights together.

[P2] The added regression passes with the old sorter unchanged.

td_new(200) has capacity 1,210, so the 50k td_add calls auto-compress in small batches rather than exercising one 50k-element partition. I linked the PR's new test file against the pre-fix library: all 21 tests / 5,152,257 assertions passed. Please add a regression that observes the complexity bound (for example, test-only comparison instrumentation), rather than relying only on correctness assertions or timing.

[P2] The “output is identical” claim does not hold for weighted duplicates.

The new equal-key permutation changes how weighted centroids are formed. In one deterministic weighted-duplicate case, the previous sorter produced 8 centroids and this PR produced 7; CDF(0) changed from 0.07999 to 0.03749, and the 0.9 quantile changed from 3.9517 to 4. If exact compatibility is not required, please remove the claim and add weighted-duplicate accuracy coverage; otherwise the tie behavior needs to be preserved.

The normal unit suite and an ASan/UBSan build pass locally, and this combines cleanly with #41.

[fcostaoliveira]

Thanks — all three addressed in 71c372b.

[P1] Guaranteed O(n log n). Replaced the 3-way quicksort with an introsort: 3-way (Dutch-flag) partitioning for equal-key runs, a median-of-three pivot, and a recursion-depth limit that falls back to heapsort, so no crafted permutation (distinct or not) can force quadratic time; stack is O(log n). Small ranges finish with insertion sort. Confirmed O(n log n) on ascending/descending/all-equal/organ-pipe/center/sawtooth/random and 20k randomized parallel-array trials.

[P2] Complexity regression that observes the bound. New ctest target td_sort_complexity_test builds the library with -DTD_INSTRUMENT_SORT (a zero-cost key-comparison counter), forces one compress over a large adversarial midpoint-killer permutation, and asserts the comparison count stays within an O(n log n) bound. On this input it does ~9.5M comparisons vs an ~5e9 quadratic count — it fails loudly if the guarantee ever regresses. (Also fixed the cmake unit-test build so ctest actually runs it — minunit needs _POSIX_C_SOURCE, hidden by -std=c99.)

[P2] "Output identical" claim dropped. You're right — the equal-key order differs, changing weighted-centroid merges, so it is not centroid-for-centroid identical. Removed the claim (results stay within t-digest's accuracy guarantee) and added test_weighted_duplicates_accuracy covering weighted duplicates (min/max/size exact; quantiles/CDF accurate and monotone).

22 unit tests / 5.15M assertions + the complexity test pass via ctest; ASan+UBSan clean.

[gabsow]

Four blocking findings remain after re-review; details are inline.

[gabsow]

TD_INSTRUMENT_SORT is also defined by target_compile_definitions() in tests/CMakeLists.txt. CMake's -DTD_INSTRUMENT_SORT defines it as 1, then this line redefines it with an empty replacement list. GCC/Clang diagnose that redefinition, and the project's ENABLE_SANITIZERS configuration promotes it to an error via -Werror, so the sanitizer CMake build fails. Please remove one of the two definitions.

[fcostaoliveira]

Fixed in c5b63b4 — guarded the in-file define with #ifndef TD_INSTRUMENT_SORT, so CMake's -DTD_INSTRUMENT_SORT=1 no longer collides with it. Verified with a clean -Werror build under -DENABLE_SANITIZERS=ON.

[gabsow]

This is not actually a killer for the old in-place midpoint/Lomuto sorter: partition swaps destroy the recursively assigned midpoint positions. Instrumenting the exact pre-PR sorter on this generated input at n=100000 gives only 1,700,396 comparisons, far below this test's 83,048,202 bound, so the old quadratic implementation would pass the claimed regression. Please use the validated adversarial permutation from the original reproduction, or explicitly instrument/assert the heapsort fallback (ideally with multi-size scaling) so removing the worst-case guarantee makes the test fail.

[fcostaoliveira]

Good catch — confirmed the midpoint-killer is not adversarial for the pre-PR Lomuto sort (measured ~1.7M comparisons at n=1e5, well under the old bound). Replaced it in c5b63b4 with two measured, multi-size regressions:

• Part A — the actual MOD-17228 vector (N identical values). The pre-PR central-pivot Lomuto sort does exactly n²/2 comparisons here (measured 5.0e9 at n=1e5); the 3-way partition does exactly 2n. The test asserts a strict linear bound (8n), which a revert to any sort without equal-run handling misses by ~3 orders of magnitude.
• Part B — the heapsort fallback, driven directly (measured ~1.78·n·log₂n), asserting an O(n log n) bound (4·n·log₂n) with the normalized ratio held flat (1.735–1.776) across n=25k…200k, so weakening/removing the fallback fails here.

All constants are measured, not assumed.

[gabsow]

This can overflow signed int for a valid large heap. With n > INT_MAX/2, sift-down can move i to a leaf above (INT_MAX-1)/2; the next 2 * i + 1 is undefined behavior and can become a negative/out-of-bounds index. #41 permits capacities approaching INT_MAX, so this is within the accepted range on large-memory hosts. Guard leaf indices before multiplying (for example, stop when i >= n / 2) or use checked wider/unsigned indices.

[fcostaoliveira]

Fixed in c5b63b4 — td_sift_down now loops while (i < n/2) and forms 2*i+1 only for internal nodes. For i < n/2, 2*i < n so 2*i+1 <= n and cannot overflow signed int, even at capacities near INT_MAX. Heapsort comparison counts are unchanged (same algorithm), verified by the Part-B measurements.

[gabsow]

td_add() currently accepts and stores non-finite means, but this sorter has no total ordering for NAN. If the pivot is NaN, both v < pivot and v > pivot are false, so every value is classified into the equal band and the range can remain unsorted; the insertion-sort path has the same issue ([2.0, NAN, 1.0] remains unordered). td_compress() then consumes nodes whose sorted invariant is false. Please either reject !isfinite(mean) in td_add() before mutation or define/document a total ordering and add NaN/±Inf coverage.

[fcostaoliveira]

Fixed in c5b63b4 — td_add() now rejects NaN with EINVAL before any mutation (matching the reference t-digest, which rejects NaN in add()), so the sort's total-order assumption holds. ±Inf remain accepted (they have a valid ordering and become min/max). Documented in tdigest.h; added test_add_nonfinite covering NaN-reject, ±Inf-accept, and the sorted invariant after a compress with infinities present.

[fcostaoliveira]

Pushed c5b63b4 addressing all four review comments:

1. Heapsort int overflow — td_sift_down tests leaf-ness (i < n/2) before forming 2*i+1, so the child index can't overflow signed int at capacities near INT_MAX.
2. NaN ordering — td_add() rejects NaN (EINVAL) before mutating, restoring the sort's total-order assumption; ±Inf still accepted. Documented + test_add_nonfinite added.
3. Invalid complexity regression — confirmed the midpoint-killer didn't defeat the pre-PR sorter (~1.7M cmps). Replaced with two measured, multi-size guards: Part A N-identical-values (pre-PR = n²/2, measured 5.0e9 @ 1e5; new = 2n → linear bound) and Part B the heapsort fallback (~1.78·n·log₂n → O(n log n) bound, flat ratio 1.735–1.776 across 25k–200k).
4. Macro redefinition — #ifndef guard so CMake's -DTD_INSTRUMENT_SORT=1 no longer collides under -Werror.

Also fixed the tests/CMakeLists.txt include path (CMAKE_CURRENT_LIST_DIR/../src) for add_subdirectory() consumers, matching the #44 fix.

Verified locally: clean -Werror build under -DENABLE_SANITIZERS=ON; full suite under ASan+UBSan 23 tests / 5,152,468 assertions / 0 failures; complexity test passes; clang-format clean.

[fcostaoliveira]

Follow-up a5a8f3c clears the SonarCloud reliability gate: moved the sort's comparison counter into an inlined td_key_lt() helper (removes the comma-operator side effect from the && operands, S912; comparison counts byte-for-byte identical), split multi-declarations (S1659), and calloc()d the complexity-test scratch buffers so the analyzer no longer traces an uninitialized read into swap_l (S836). Quality gate is green.

[gabsow]

Two blockers remain after re-review; details are inline.

[gabsow]

+/-Inf has a total order for sorting, but it is not closed under the centroid-merge arithmetic. Reproducer against a5a8f3c: create td_new(200), add INFINITY with weight 1 one hundred times, then call td_compress(); this produces a NaN centroid (I observed index 17 of 61), because the merge computes delta = Inf - Inf and then adds NaN at lines 770–772. A later sort therefore loses its ordering invariant again even though direct NaN input is rejected. Please either reject all non-finite means or explicitly handle equal infinite centroid merges, and add repeated +/-Inf coverage.

[fcostaoliveira]

Confirmed and fixed in d8cd91d. Reproduced exactly (100x +Inf -> 26 NaN centroids, first at index 17/61): the merge computes delta = Inf - Inf = NaN. td_add() now rejects all non-finite means (!isfinite), not just NaN, so every stored mean stays finite and the merge can never manufacture a NaN. Header updated; test_add_nonfinite now asserts NaN/+Inf/-Inf are all rejected and that the repeated-+Inf reproducer leaves the digest empty.

[gabsow]

This exercises td_heap_sort() in isolation, not the introsort fallback integration. I removed the depth_limit == 0 fallback branch locally while leaving this helper intact; both Part A and Part B still passed every size and bound. Therefore the test does not catch removal of the claimed worst-case O(n log n) guarantee. Please drive a validated adversarial input through the full td_qsort() path, instrument/assert that the fallback is actually reached, and keep the multi-size comparison bound.

[fcostaoliveira]

Good catch — you're right that the isolated td_heap_sort() test could not detect fallback removal. Rewrote Part B in d8cd91d to drive a validated adversarial input through the full td_qsort() path: it generates a median-of-three killer with McIlroy's quicksort adversary run against a faithful mirror of this introsort's comparison sequence, feeds it through the real td_compress(), and asserts across sizes that (a) the fallback is actually reached (new td_sort_heap_fallbacks counter, measured 1 per size), (b) comparisons stay O(n log n) (~5.6 n·log₂n), and (c) the ratio is flat. I verified both failure modes: driving the same killer through a no-fallback quicksort (depth_limit=INT_MAX) is quadratic — 5.0e9 comparisons at n=1e5 (~3000 n·log₂n) — so removing the fallback fails both the fallback-reached assert and the bound. Part B uses modest sizes because killer generation is itself O(n²).

[fcostaoliveira]

Follow-up 68999a7 clears the SonarCloud reliability gate (calloc the killer-generation buffers so the analyzer no longer reports an uninitialized read into the mirror comparator; no behavior change). Gate is green.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[gabsow]

Verified the latest fixes: duplicate-heavy and adversarial sort regressions pass, non-finite inputs are rejected before mutation, and all CI checks are green.

[gabsow]

Superseded by #45, which contains the complete reviewed #42 change set rebased onto the post-#44 master and resolves the shared CMake/test conflicts. The combined tree passed GCC, Clang, ASan+UBSan, SonarCloud, Hound, and the full 3-test CTest suite. It was merged as daf1f4a. Closing this fork PR because GitHub rejected maintainer updates to its head branch with HTTP 403.