Repository navigation
Conversation
KllSketch::update_with_weight(item, weight) is equivalent to calling update(item) weight times. A weight that fits into the free capacity 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>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The randomized accuracy test can fail legitimately, and the complexity documentation overstates the implementation guarantee.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Adds efficient weighted updates to KLL sketches.
Changes:
- Adds
update_with_weightand internal merge refactoring. - Adds weighted-update integration tests and changelog documentation.
| File | Description |
|---|---|
datasketches/src/kll/sketch.rs |
Implements weighted updates. |
tests-integration/tests/kll_test/update.rs |
Tests weighted-update behavior. |
tests-integration/tests/kll_test/main.rs |
Registers the new tests. |
CHANGELOG.md |
Documents the API. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+114
to
+115
| (rank - true_rank).abs() <= RANK_EPS_FOR_K_200, | ||
| "item {item}: rank {rank}, true rank {true_rank}" |
| ### New features | ||
|
|
||
| * `KllSketch` is now available behind the `kll` feature, with rank, quantile, PMF, and CDF queries, merging, serialization, custom ordered item types, and a `KllFloat` adapter for non-NaN floating-point values. | ||
| * `KllSketch` is now available behind the `kll` feature, with rank, quantile, PMF, and CDF queries, merging, serialization, custom ordered item types, and a `KllFloat` adapter for non-NaN floating-point values. `KllSketch::update_with_weight` adds an item repeated a given number of times, at a cost logarithmic in the weight. |
Comment on lines
+148
to
+150
| /// The result is equivalent to calling [`update`](Self::update) `weight` times, at a cost | ||
| /// that grows with the logarithm of `weight` rather than with `weight` itself. A weight of | ||
| /// zero is a no-op. |
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.


Adds
KllSketch::update_with_weight(item, weight), equivalent to callingupdate(item)weighttimes. This mirrorsupdate(item, weight)on the KLL sketches in datasketches-java. The name followsCountMinSketch::update_with_weight.Behavior
weightis smaller than the free capacity, the item is insertedweighttimes as plain updates. No compaction can happen on this path.hcounts as2^h, the exact sketch holds one copy of the item at each level whose bit is set inweight. Bits above the top level fold intoweight >> 60copies at level 60, because level capacities are only defined up toMAX_NUM_LEVELS(61). This path costs one merge with a sketch of at most 75 items, however large the weight.FrequentItemsSketch::update_with_count.u64::MAX, it panics without modifying the sketch, the same asupdate.merge's body moves into a privatemerge_unchecked, so the weighted path can skip checks it has already done without discarding aResult.mergeis otherwise unchanged.Tests
New module
tests-integration/tests/kll_test/update.rs:2^40 + 12345weightu64::MAX - 1weight, with a serialization round tripcargo x test,cargo clippy --all-features --all-targets --workspace -- -D warningsandcargo +nightly fmt --all --checkpass locally. I did not run taplo, typos or hawkeye locally, so CI will be their first run.Companion PR for datasketches-cpp: apache/datasketches-cpp#542
🤖 Generated with Claude Code