fix: reject sketch weight overflow before mutation - #282
Merged
Merged
Conversation
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
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.
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
n + 1on update andn + other.non merge.InvalidArgument.InvalidArgument.Count-Min needs two related boundary rules:
|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:
u8only holds 255 total units,i8holds 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
u64sketches, 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 reachu64::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
37b4b1dand C++70e462f:Math.addExact; heap Items update increments normally.Math.addExact.Unchecked code is an implementation observation, not a documented promise that wrapping results remain meaningful. Java's signed
longlimits also differ from Rust'su64limits. 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), andcargo x lintpass.--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.DEVELOPER_DIR=/Library/Developer/CommandLineToolsbecause 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, rustc1.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):
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 results show no severe regression in these workloads, but do not establish zero overhead or performance on other hardware. The per-centroid
NonZeroU64::checked_addin 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.