fix: prevent Summary quantiles from collapsing to the minimum observation - #2316
fix: prevent Summary quantiles from collapsing to the minimum observation#2316manduinca wants to merge 1 commit into
Conversation
…tion CKMSQuantiles returned the minimum observation for every targeted quantile whenever 2*epsilon >= 1-quantile (e.g. quantile(0.9, 0.05) or quantile(0.99, 0.005)). At the boundary the error function permits a sample's uncertainty (delta) to reach the order of n at low ranks, which broke both the query and compression: - get() stopped at the first sample whose maximum rank exceeded desiredRank + f(desiredRank)/2 and returned the preceding sample. A freshly inserted low-rank sample (delta = f(r) - 1) then made the scan stop almost immediately, returning a value near the minimum. get() now returns the sample whose possible-rank interval is centered closest to the desired rank. - compress() bounded merges by the error function at the left edge only, where f is huge for low ranks, so it merged away all resolution between the median and the maximum. Merges are now bounded by the error function over the whole rank interval the merged sample would span, via a new f(lo, hi) overload (f(r) delegates to f(r, r), so single-rank behavior is unchanged). Adds regression tests for the boundary configuration. Fixes prometheus#2292 Signed-off-by: Jean Pierre Mandujano G. <jeanpierre.mandujano@gmail.com>
zeitlinger
left a comment
zeitlinger
left a comment
There was a problem hiding this comment.
Thanks for the focused reproducer. I found a correctness issue in the new rank-selection heuristic, plus a couple of changes that would make the fix easier to review:
-
get()currently chooses the sample whose possible-rank interval has the nearest center. That does not preserve the advertised rank bound. With values1..10_000shuffled usingnew Random(2), a single(q=0.99, epsilon=0.005)configuration returns9784; the allowed range under the test's2 * epsilonbound is[9800, 10000]. The sample centered nearest to rank 9900 is therefore not a sufficient selection criterion. Please rework the selection against the CKMS error bounds and add this smaller deterministic regression case. -
Please rename
f. It is now overloaded for both a point rank and an interval, but the name gives no indication of either the mathematical meaning or the interval semantics. Descriptive names such aserrorBoundAtRankandminErrorBoundInRangewould make the compression logic much easier to audit. Please update the associated Javadocs and test references too. -
The new tests only use
n=100_000, and their broad2 * epsilonassertions do not verify the comment's claim that the quantiles remain distinct or exercise the2 * epsilon > 1 - quantilecase. Please add direct rank-bound coverage for the failing smaller case and a strict-above-boundary case.
The two new boundary tests pass, but the additional deterministic case above fails on the current head.
…on >= 1-quantile Fixes prometheus#2292. CKMSQuantiles returned values from far below the requested quantile for quantile configurations such as (0.9, 0.05) or (0.99, 0.005) - often the minimum of all observations, regardless of the input data. Interacting root causes, all stemming from the error function f() being of order n-r below a target quantile when 2*epsilon >= 1-quantile: 1. compress(): a single sample was allowed to span all ranks from r to n, so compress() merged away the samples that hold the information needed to answer the quantile query. With quantiles {(0.9, 0.05), (0.99, 0.005)} the sample list collapsed to 3 samples. 2. insertBefore(): freshly inserted samples get delta = f(r) - 1, so below a target their possible-rank intervals are centered near rank n regardless of the sample's actual position, making them indistinguishable from genuine samples near the target. 3. get(): the scan stopped at the first sample with r + g + delta > desiredRank + f(desiredRank)/2 and returned the value of the sample before it; a single wide sample (see 2., and get() flushes the buffer right before scanning, so such samples are always present) made the scan stop far before the target rank. The fix bounds sample widths by maxWidthNotCrossingTargets(r) in addition to f(r) at both places where widths are created - merging in compress() and delta assignment in insertBefore() - so that every target quantile keeps enough resolution around its accuracy window [quantile*n - epsilon*n, quantile*n + epsilon*n]. The bound is anchored at the window's start with a floor of 2*epsilon*n so that it does not degenerate for targets with quantile + epsilon >= 1 (window end == n), e.g. (0.99, 0.01) or (0.95, 0.05). get() returns the value of the sample whose possible rank interval is centered closest to the desired rank, which cannot be derailed by a single wide sample. Verified against exact percentiles on 3720 test cases (31 quantiles across 13 configurations x 6 distributions x 2 sizes x 10 seeds): worst rank error 1.75 * epsilon, no case above 2 * epsilon. Before the fix the worst rank error was 330 * epsilon. Also includes the deterministic regression case from the review of PR prometheus#2316 (values 1..10,000 shuffled with seed 2, single quantile (0.99, 0.005)), which this fix passes, plus regression tests for the quantile + epsilon >= 1 family and for descending input order. Signed-off-by: Oleg Kovalenko <okovalenko@evolution.com>
|
Heads-up: I've opened #2396 for the same issue (#2292). It shares the two-part diagnosis from this PR — the compress() merge bound and the early-stopping scan in get() — but bounds sample widths differently (window-anchored cap, applied at both merge and insert time) and keeps f() unchanged. The deterministic case from the review here (values 1..10,000, Random(2), single (0.99, 0.005)) is included #2396 as a regression test and passes (returns 9940, within [9800, 10000]), along with the strict-above-boundary coverage the review asked for. Details and the comparison harness are linked in the PR description. Happy to have either PR land — flagging it mainly so the two efforts don't diverge silently. |
3 participants
Fixes #2292
When a targeted quantile has
2*epsilon >= 1 - quantile, all reported quantiles could collapse to the minimum observation. The query scan stopped too early: a freshly inserted low-rank sample can carry adeltaon the order ofn, so the runningrank + g + deltaexceeded the target far sooner than it should and returned a value near the minimum.The fix has two parts, matching the diagnosis in the issue.
compress()now keeps a merged sample from spanning a quantile's target rank, and the query uses the minimum of the error function over a sample's rank interval[lo, hi]rather than at a single point. To support that I split the single-argumentf(r)intof(lo, hi)(the plain error function isf(r, r)), which is numerically identical for the existing call sites.Added a regression test that reproduces the collapse and asserts distinct quantiles.
CKMSQuantilesTestpasses (18 tests).