fix(cms): stop clamping RegularPath estimates at i32::MAX#76
Merged
Conversation
CountMin::estimate on the regular path seeded the running minimum with S::Counter::from(i32::MAX). For counters wider than i32 (i64/i128), any key whose probed cells all exceed 2147483647 never triggered the `v < min` update, so estimate silently returned the seed — capping every large count at exactly 2147483647. Seed the minimum from row 0's probed cell instead, matching the fast path's fast_query_min. Add a regression test covering an i64 sketch with counts of ~35 billion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Problem
CountMin::estimateon the regular path seeds the running minimum withS::Counter::from(i32::MAX):For counters wider than
i32(i64/i128), any key whose probed cells all exceed2147483647never fires thev < minupdate, soestimatereturns the seed unchanged — silently capping every large count at exactly2147483647.This bites real workloads: e.g.
CMSHeap<Vector2D<i64>, RegularPath>over aSUM(pkt_len)window where true per-key totals run into the tens of billions. Every such key comes back as2147483647.Scope
fast_query_minalready seeds from row 0.RegularPath(the struct defaultMode); it delegates tocms.estimate.f64, no constant seed.The bug only manifests with counts
> i32::MAXand a counter type wider thani32, which is why existing tests (small counts /i32counters) never caught it.Fix
Seed the running minimum from row 0's probed cell, then fold in rows
1..rows— mirroring the fast path'sfast_query_min. No trait-bound or API changes.Test
Adds
estimate_does_not_clamp_i64_counts_above_i32_max: ani64sketch loaded with ~35 billion and asserting the estimate is not clamped. Fails on the old seed, passes now. Fullcountminsketchsuite (64 tests) green.🤖 Generated with Claude Code