Skip to content

Add weighted update to kll_sketch - #542

Open
shanielh wants to merge 2 commits into
apache:masterfrom
shanielh:kll-weighted-update
Open

shanielh wants to merge 2 commits into
apache:masterfrom
shanielh:kll-weighted-update

Conversation

@shanielh

@shanielh shanielh commented Oct 7, 2026 •

Copy link
Copy Markdown

Adds kll_sketch::update(item, weight), equivalent to calling update(item) weight times. This mirrors update(item, weight) on the KLL sketches in datasketches-java.

Behavior

  • If weight is smaller than the free space in level zero, the item is inserted weight times as plain updates. No compaction can happen on this path.
  • Otherwise, the update builds an exact sketch of the weighted item and merges it in. Since an item at level h counts as 2^h, the exact sketch holds one copy of the item at each level whose bit is set in weight. Bits above level 60 fold into weight >> 60 copies at level 60, because level capacities are only defined up to 61 levels. This path costs one merge with a sketch of at most 75 items, however large the weight.
  • A weight of zero throws std::invalid_argument, the same as in Java. NaN items are ignored, as in update(item).
  • Serialization is unchanged, so a sketch built with weighted updates stays readable by every other implementation.

Fix included

The first commit fixes an infinite loop in kll_helper::floor_of_log2_of_fraction. Once numer >= 2^63, doubling denom overflows to zero before it exceeds numer. ub_on_num_levels(n) calls this function with the merged stream weight, so any merge that reaches n >= 2^63 used to hang. A weighted update makes that weight easy to reach. The fix compares denom against numer / 2, which gives the same result without the overflow. The fix has its own unit cases.

Tests

New sections in kll_sketch_test.cpp:

  • zero weight throws
  • NaN is ignored
  • matches repeated updates exactly in exact mode
  • a single item with a 2^40 + 12345 weight
  • UINT64_MAX - 1 weight, with a serialization round trip
  • large weights merged into a full sketch
  • 10,000 weighted items in estimation mode, ranks within the k=200 error bound, with a serialization round trip
  • strings

The kll test suite passes on clang (macOS arm64) in a plain debug build and with -fsanitize=address,undefined.

Companion PR for datasketches-rust: apache/datasketches-rust#293

🤖 Generated with Claude Code

shanielh and others added 2 commits October 7, 2026 14:09
…numerators

Doubling denom until it exceeds numer overflows to zero once numer >= 2^63,
so the loop never terminated. ub_on_num_levels(n) calls it with the merged
stream weight, which made a merge reaching n >= 2^63 hang. Compare against
numer / 2 instead, which is equivalent and cannot overflow.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
update(item, weight) is equivalent to calling update(item) weight times.
A weight that fits into the free space of level zero is applied as plain
updates; a larger one builds an exact sketch holding one copy of the item
at each level whose bit is set in the weight and merges it, so the cost is
logarithmic in the weight. This mirrors the weighted update in
datasketches-java.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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