MOD-17228: Fix O(n²) t-digest compress on duplicate input - #45
Merged
Conversation
td_qsort used a single central pivot and recursed both partitions. On a run of equal keys the partition peels one element per level -> O(n^2) comparisons and O(n) recursion depth. Duplicate/low-cardinality input is t-digest's common case (it summarizes streams of repeated measurements), so this is not an adversarial edge: a compress over N buffered equal values is quadratic. Because compress runs inline on the calling thread, a large such compress stalls that thread for the full O(n^2). Measured through the RedisBloom TDIGEST commands on Redis 8.8.1: 430k identical values -> a single TDIGEST.QUANTILE (which triggers compress) blocks the server ~98s. Replace the sort with 3-way (Dutch-national-flag) partitioning plus tail-recursion elimination: equal keys collapse in a single pass (O(n log n) even for all-equal input) and recursing only the smaller side bounds the stack to O(log n). Output is identical to the previous sort. Single compress of N identical values, before -> after: 50k: 1,238 ms -> 1.0 ms 100k: 4,994 ms -> 2.0 ms 200k: 20,069 ms -> 3.2 ms 5M: (hours) -> 80 ms Verified: existing unit suite passes unchanged, plus a new test (all-equal / ascending / descending, 50k each) asserting sorted centroids and correct quantiles; 20,000 randomized parallel-array sort trials; ASan/UBSan clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review on #42: [P1] A deterministic midpoint pivot still permits a crafted DISTINCT-value permutation to force O(n^2). Replace the 3-way quicksort with an introsort: 3-way (Dutch-flag) partitioning for duplicate runs, a median-of-three pivot, and a recursion-depth limit that falls back to heapsort -- guaranteeing O(n log n) worst case and O(log n) stack for any input. Small ranges finish with insertion sort. [P2] Add a complexity regression that OBSERVES the bound instead of relying on correctness/timing: build the library with TD_INSTRUMENT_SORT (a key-comparison counter, zero-cost otherwise), force ONE large compress over an adversarial midpoint-killer permutation, and assert the comparison count stays within an O(n log n) bound (a quadratic sort would need ~n^2/2). New ctest target `td_sort_complexity_test`. Also fixes the cmake unit-test build so ctest runs (minunit needs _POSIX_C_SOURCE, hidden by -std=c99). [P2] The "output is identical" claim was wrong for weighted duplicates: the equal-key order differs, changing how weighted centroids merge. Drop the claim (the sort stays within t-digest's accuracy guarantee) and add `test_weighted_duplicates_accuracy` asserting correct quantiles/CDF/min/max/size. Verified: 22 unit tests / 5.15M assertions + complexity test pass via ctest; ASan+UBSan clean; introsort O(n log n) confirmed on ascending/descending/ all-equal/organ-pipe/center/sawtooth/random over 20k randomized parallel-array trials; clang-format OK. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- td_sort_complexity_test: free the histogram on the early error-return paths (S3584 potential leak of 'h'). - td_test: use an integer loop counter in the CDF sweep instead of a float (S2193 "do not use a counter of type float"). ctest green; LSan clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ssion Address gabsow's second review on #42: - td_sift_down: `2*i + 1` could overflow signed int for a valid large heap (#41 permits capacities near INT_MAX). Test leaf-ness (`i < n/2`) before forming the child index; for i < n/2, 2*i+1 <= n and cannot overflow. - td_add: reject NaN (EINVAL) before mutating. The centroid sort assumes a total order over means, but NaN compares false to everything and would leave a partition unsorted, breaking td_compress()'s invariant. Matches the reference t-digest; +/-Inf remain accepted (valid ordering). Documented in tdigest.h; added test_add_nonfinite covering NaN-reject / Inf-accept / sorted. - td_sort_complexity_test: the distinct "midpoint killer" was not adversarial for the pre-PR Lomuto sort (partition swaps destroy the midpoint layout; ~1.7M comparisons, under the old bound), so a quadratic revert would pass it. Replace with two measured, multi-size regressions: Part A sorts N identical values -- the actual MOD-17228 vector, where the pre-PR sort is exactly n^2/2 and the 3-way partition is exactly 2n -- and asserts a linear bound; Part B drives the heapsort fallback directly (~1.78 n log2 n) and asserts an O(n log n) bound with a flat normalized ratio across sizes. - Fix the macro redefinition: the file `#define TD_INSTRUMENT_SORT` clashed with CMake's `-DTD_INSTRUMENT_SORT=1`, which -Werror (ENABLE_SANITIZERS) rejects. Guard it with `#ifndef`. - tests/CMakeLists.txt: use CMAKE_CURRENT_LIST_DIR/../src so the include path survives add_subdirectory() consumption. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Move the sort's comparison counter into an inlined td_key_lt() helper so the boolean conditions no longer carry a comma-operator side effect in the && RHS (S912). Inlines to a plain `a < b` when TD_INSTRUMENT_SORT is off; comparison counts are byte-for-byte identical (verified: all-equal 2n, heapsort ~1.78 n log2 n unchanged). - Split multi-declarations into dedicated statements (S1659). - calloc() the complexity test's scratch buffers so the analyzer no longer traces an uninitialized read through td_heap_sort -> swap_l (S836 BUG, the sole new_reliability_rating failure). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address gabsow's third review on #42: - td_add: reject all non-finite means, not just NaN. +/-Inf sorts fine but is not closed under the centroid merge: merging two equal infinities computes delta = Inf - Inf = NaN, poisoning a centroid mean and re-breaking the sort invariant (reproduced: 100x +Inf -> 26 NaN centroids). Rejecting !isfinite at ingest keeps every stored mean finite. Header + test_add_nonfinite updated to assert +/-Inf are rejected and the repeated-+Inf reproducer stays empty. - Complexity test Part B rewritten. The old helper drove td_heap_sort() in isolation, so removing the depth-limit fallback did not fail it. It now 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()/td_qsort(), and asserts across sizes that the heapsort fallback is actually reached (new td_sort_heap_fallbacks counter), the comparison count stays O(n log n) (measured ~5.7 n log2 n; the same input without the fallback is ~n^2/2, ~3000 n log2 n at n=1e5), and the ratio is flat. Removing the fallback fails both the fallback-reached and bound asserts. Part B uses modest sizes because killer generation is itself O(n^2). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The analyzer cannot prove the malloc'd index array is fully initialized before it is read in the mirror sort's comparator, flagging an uninitialized-read BUG (the sole new_reliability_rating failure). calloc the generation buffers; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # tests/CMakeLists.txt
…t-duplicates # Conflicts: # tests/CMakeLists.txt # tests/unit/td_test.c
|
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
2 participants
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.
Replaces #42 because its cross-repository head branch could not be updated after #44 landed and introduced conflicts in the shared test registration files.
This preserves the reviewed #42 implementation and combines it with current
master:td_free(NULL)hardening merged in test: extend td_init coverage + guard td_free(NULL) (follow-up to #41) #44;Validation:
Supersedes #42 with no functional changes beyond resolving its conflicts with #44.
Note
Medium Risk
Core
td_compresssorting andtd_addvalidation change behavior on duplicate-heavy and adversarial inputs; equal-key tie order can differ from the old sorter though accuracy is preserved, and callers passing NaN/Inf now get EINVAL instead of silent corruption.Overview
Fixes MOD-17228 by replacing the centroid Lomuto quicksort in
td_compress()with introsort: 3-way (Dutch-flag) partitioning for duplicate-heavy keys, median-of-three pivots, insertion sort on small ranges, recursion on the smaller partition only, and a depth-limited heapsort fallback so worst-case time stays O(n log n). OptionalTD_INSTRUMENT_SORTcounters support a dedicated complexity regression test.td_add()now returnsEINVALfor non-finite means (NaN and ±Inf) before mutating the digest, so sort order and centroid-merge math stay valid;tdigest.hdocuments the contract.Tests add
td_sort_complexity_test(linear bound on all-equal input, O(n log n) bound plus heapsort fallback on an adversarial permutation), plus unit coverage for non-finite rejection, large duplicate-heavy compress paths, and weighted-duplicate quantile accuracy.Reviewed by Cursor Bugbot for commit cdaf355. Bugbot is set up for automated code reviews on this repo. Configure here.