Skip to content

fix: reject sketch weight overflow before mutation - #282

Merged
tisonkun merged 4 commits into
apache:mainfrom
tisonkun:codex/sketch-weight-overflow
Sep 26, 2026
Merged

tisonkun merged 4 commits into
apache:mainfrom
tisonkun:codex/sketch-weight-overflow

Conversation

@tisonkun

@tisonkun tisonkun commented Sep 26, 2026 •

Copy link
Copy Markdown
Member

Total-weight overflow can silently corrupt sketch state in release builds: a merged REQ sketch can become empty, a T-Digest can retain data with a wrapped total, and a small-integer Count-Min sketch can wrap after only a few weighted updates. KLL already rejects overflow, but its update check runs after changing extrema or compacting retained items. This change rejects overflowing operations before modifying the sketch, using checks at the total-weight boundary.

This follows #281 and its discussion of the Java frequent-items emptiness fix. The broader audit found that preserving a meaningful total weight also matters beyond frequent items; this PR starts from main after #281 was merged.

Behavior and scope

Sketch Change Overflow behavior
KLL Move the existing update check ahead of extrema changes and compaction. The internal insertion path relies on the public update/merge checks. Update panics; merge already returns an error.
REQ Check n + 1 on update and n + other.n on merge. Update panics; merge returns InvalidArgument.
T-Digest Check the combined compressed and buffered weight before update/merge changes extrema or takes the buffer. Update and merge panic, preserving their existing signatures.
Count-Min Check the accumulated absolute weight on update/merge, including signed minimum values whose magnitude is unrepresentable. Update panics; merge returns InvalidArgument.

Count-Min needs two related boundary rules:

  • Every bucket satisfies |count| <= total_weight. For a merge, |a + b| <= |a| + |b| <= W_left + W_right <= T::MAX, after checking the combined total once. Updates use the same argument with |weight|. Deserialization validates the invariant, rejecting negative totals and out-of-range bucket magnitudes; unsigned halving and decay preserve it. The bucket loops keep ordinary arithmetic, and valid negative counters remain supported.
  • upper_bound() clamps to the counter type's maximum if adding the error allowance would overflow. This is a query bound, not saturation of stored counters or weights. Cancellation of signed updates still consumes absolute weight.

No public signatures change. The implementation diff spans five files and adds 49 net lines including API documentation. T-Digest passes the checked combined weight directly into compression, and sorting is inlined at its single call site. The boundary regressions live in the existing sketch test modules. This does not add checks to every internal addition, introduce a shared overflow framework, or constrain Tuple's caller-defined summary arithmetic. Tuple's default policies intentionally delegate to AddAssign; a universal checked-arithmetic requirement would change that generic contract. Frequent items is already handled by #281. The bounded retained-state/register counters in the other sketch families are outside this total-stream-weight change.

The Unreleased changelog names each affected sketch first and keeps its entries adjacent within each change category. Overflow and deserialization behavior are documented separately for each sketch, with library-wide changes listed separately.

Is this worth checking?

The practical case is strongest for Count-Min: u8 only holds 255 total units, i8 holds 127 absolute units, and explicit weights can reach the limit immediately. Applications can choose a wider type or use the existing unsigned decay/halving operations.

For the u64 sketches, ordinary unit updates are extremely unlikely to reach the limit: even one billion updates per second would take about 585 years. Merging changes that calculation. Starting with one item, 63 rounds of doubling and adding one reach u64::MAX; one more item or nonempty merge then overflows. These are valid public operations, though repeated doubling is primarily a boundary test and could reflect accidental repeated aggregation rather than a typical production workload. The tests do not require forged serialized data to reach this state.

The recommendation is to keep these few boundary checks because overflow destroys the interpretation of the whole sketch, while avoiding a general campaign to check every arithmetic operation. The performance results below do not establish that the checks are free.

Java and C++ comparison

Upstream does not have a uniform "let it overflow" policy. These observations are pinned to Java 37b4b1d and C++ 70e462f:

Sketch Java C++
KLL Items merge uses Math.addExact; heap Items update increments normally. Update and merge have no explicit overflow check.
REQ Merge and update use unchecked arithmetic. Update and merge use unchecked arithmetic.
T-Digest Both update and merge use Math.addExact. Compressed weight is accumulated without an explicit check.
Count-Min Update and merge use unchecked arithmetic. Update and merge use unchecked arithmetic.

Unchecked code is an implementation observation, not a documented promise that wrapping results remain meaningful. Java's signed long limits also differ from Rust's u64 limits. This PR follows Rust's existing method signatures for panic versus returned error and checks before any state mutation.

Validation and performance

  • cargo x check, cargo x test (696 tests including doctests and serialization compatibility), and cargo x lint pass.
  • Six focused overflow regressions also pass under --release. Tests cover reaching the exact limit through public operations, rejecting the next update/merge without changing state, pending T-Digest buffers and extrema, Count-Min signed minimum/cancellation, and upper-bound clamping. A serialization regression rejects Count-Min states that would invalidate the arithmetic invariant.
  • Local commands used DEVELOPER_DIR=/Library/Developer/CommandLineTools because the selected Xcode installation required license acceptance; no machine configuration was changed.

The final code was compared with main (7f4c145) on an Apple M4 Max, rustc 1.99.0-nightly (3d6c19bb9), in release mode. Both binaries used the same temporary Divan harness and the system allocator, without an allocation profiler. There were 29 workloads, three runs per binary in before/after/after/before/before/after order, and at least 0.5 seconds per workload per run. The harness reused the repository's update and merge benchmarks and supplemented Count-Min signed updates, merge/deserialization, and REQ merge. Supplemental Count-Min cases use four hashes and either 16 or 16,384 buckets. The harness and logs remain outside the repository.

Representative median-of-run-medians (the comparison measures the whole PR, not an isolated instruction):

Workload Main This PR Change in time
Count-Min u64 update, 10,000 inputs 94.29 us 93.83 us -0.5%
Count-Min i64 weighted update, 10,000 inputs 89.29 us 89.62 us +0.4%
KLL update, 1,000 inputs 9.041 us 9.165 us +1.4%
REQ update, 100,000 inputs 1.920 ms 1.871 ms -2.6%
T-Digest update, 100,000 inputs 2.619 ms 2.651 ms +1.2%
Count-Min merge, 64 counters 14.33 ns 15.53 ns +8.4%
Count-Min merge, 65,536 counters 10.70 us 10.66 us -0.4%
T-Digest merge, compressed inputs 2.041 us 2.083 us +2.1%
Count-Min i64 deserialize, 64 counters 252.3 ns 265.4 ns +5.2%
Count-Min i64 deserialize, 65,536 counters 220.1 us 218.2 us -0.9%

The sub-microsecond cases and compressed T-Digest merge were also repeated with a fixed sample size of 32, at least one second per case, in before/after/after/before order:

  • The tiny Count-Min merge consistently adds about 1.2–1.3 ns. Its relative overhead is 8–12% depending on sampling; the larger merge remained effectively unchanged. The single check has a measurable fixed cost.
  • Compressed T-Digest merge remained about 1–2% slower (roughly 30–40 ns).
  • The small Count-Min signed-deserialization increase was not stable: repeat median ranges overlapped, 253.6–267.9 ns before versus 252.4–254.9 ns after. Large deserialization cases also overlapped. Validation still performs work proportional to the number of counters.

The results show no severe regression in these workloads, but do not establish zero overhead or performance on other hardware. The per-centroid NonZeroU64::checked_add in T-Digest predates this PR and remains; replacing it would require another nonzero construction check or unsafe code. KLL no longer repeats its overflow check for every level-zero item inserted during merge. Count-Min bucket loops retain their original arithmetic, with one total-weight check before the loop.

@tisonkun
tisonkun merged commit 003ce7a into apache:main Sep 26, 2026
10 checks passed
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.

1 participant