Skip to content

fix(cms): fold column index in queryFrequencyFast to match insert (non-pow2 cols) - #60

Merged
zzylol merged 1 commit into
mainfrom
fix/cms-query-fold
May 26, 2026
Merged

zzylol merged 1 commit into
mainfrom
fix/cms-query-fold

Conversation

@zzylol

@zzylol zzylol commented May 26, 2026

Copy link
Copy Markdown
Collaborator

Problem

In sketches/CountMinSketch/CountMinSketch.go, the insert path folds the per-row column index back into range when cols is not a power of two:

c := int((hash >> shift) & s.mask)
if c >= s.Cols { c %= s.Cols }   // InsertWithHash, ~L229-231

but the query fast path queryFrequencyFast (~L354-357) indexed RowSlice(r)[c] with no fold. Because hashLayoutForCols rounds the mask up to the next power of two, a masked index can exceed cols, so querying a non-power-of-two CMS could panic (index out of range) and/or read the wrong cell. queryFrequencyFast was the only unfolded read site (estimateMatrixHash, QuerySum, QuerySum2 already fold).

This also matters for cross-language parity: the Rust asap_sketchlib CMS folds on both insert and query, so Go was the odd one out for non-pow2 cols.

Fix

Apply the identical fold in queryFrequencyFast so insert and query always land on the same cell.

Test

TestCMS_NonPowerOfTwoCols_QueryFold (cols = 17 and 2000, known counts):

  • Before: panic: index out of range [26] with length 17 at CountMinSketch.go:357.
  • After: PASS — QueryWithHash == FastEstimateWithHash, estimates ≥ true counts.

go build ./... clean; go test ./... passes (pre-existing CocoSketch / cross_language-needs-XTEST_DIR failures are unrelated, confirmed on untouched main).

🤖 Generated with Claude Code

The Count-Min Sketch INSERT path folds the per-row column index into
range via `c %= s.Cols` (CountMinSketch.go:229-231), but the fast
point-query path `queryFrequencyFast` (reached via QueryFrequency and
FastEstimateWithHash) indexed `RowSlice(r)[c]` WITHOUT the same fold.

For a non-power-of-two `cols`, the column mask spans the next power of
two, so a masked index can exceed `cols`. Without the fold the query
either panics (index out of range) or reads a different cell than insert
wrote, breaking the CMS one-sided over-estimate guarantee. Insert and
query disagreed on column placement.

This applies the identical fold to queryFrequencyFast so insert and
query always land on the same cell, matching the Rust asap_sketchlib
CMS (which folds on both insert and query) and the already-correct
estimateMatrixHash / QuerySum / QuerySum2 paths.

Adds a regression test using non-power-of-two cols (17 and 2000) that
inserts keys with known counts and asserts no panic and estimates >=
true counts.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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