feat: implement count-min sketch #38
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a Count-Min sketch data structure for approximate frequency counting in streaming data. The implementation provides configurable accuracy through relative error and confidence parameters, supports negative weights, and includes serialization/deserialization compatibility with the DataSketches format.
Key changes:
- Core Count-Min sketch with update, estimate, merge, and bound calculation operations
- Helper functions for parameter suggestions based on desired error and confidence levels
- Serialization support compatible with DataSketches Count-Min format
- Comprehensive test suite covering basic operations, edge cases, and serialization
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Adds countmin module to the library's public API |
| src/countmin/mod.rs | Module definition with documentation and public exports |
| src/countmin/sketch.rs | Core Count-Min sketch implementation with update, estimation, merge, and serialization methods |
| src/countmin/serialization.rs | Serialization constants and helper functions for DataSketches format compatibility |
| tests/countmin_test.rs | Comprehensive test suite covering initialization, updates, bounds, merge, serialization, and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if confidence == 1.0 { | ||
| return 127; |
There was a problem hiding this comment.
The magic number 127 returned when confidence is 1.0 lacks explanation. This appears to be chosen as a practical upper limit (since num_hashes is u8 and 127 is the max signed byte value), but it would be clearer to either use u8::MAX (255) if that's the intent, or document why 127 specifically is chosen. If 127 is a deliberate choice related to the algorithm's behavior, this should be documented in a comment.
| (0.0..=1.0).contains(&confidence), | ||
| "confidence must be between 0 and 1.0 (inclusive)" |
There was a problem hiding this comment.
The documentation states that the function panics if confidence is not in (0, 1], which means 0 should be excluded. However, the assertion uses a closed interval [0.0, 1.0] which includes 0. This creates an inconsistency between the documentation and implementation. If confidence is 0.0, the function would not panic but would cause a division by zero at line 123 (1.0 / (1.0 - 0.0) = 1.0 / 1.0 = 1.0, which is fine, but conceptually a confidence of 0 doesn't make sense for this algorithm). The documentation should either be corrected to say [0, 1] or the assertion should exclude 0.0.
| (0.0..=1.0).contains(&confidence), | |
| "confidence must be between 0 and 1.0 (inclusive)" | |
| confidence > 0.0 && confidence <= 1.0, | |
| "confidence must be in (0, 1.0]" |
| /// | ||
| /// Panics if `relative_error` is negative. | ||
| pub fn suggest_num_buckets(relative_error: f64) -> u32 { | ||
| assert!(relative_error >= 0.0, "relative_error must be at least 0"); |
There was a problem hiding this comment.
When relative_error is 0.0, the function will compute E/0.0, resulting in infinity. Converting infinity to u32 via 'as' casting results in saturating behavior, which may produce unexpected results. Consider adding validation to reject a relative_error of 0.0, or document this edge case behavior.
| assert!(relative_error >= 0.0, "relative_error must be at least 0"); | |
| assert!(relative_error >= 0.0, "relative_error must be at least 0"); | |
| if relative_error == 0.0 { | |
| // For zero relative error, return the maximum possible number of buckets. | |
| // This preserves the previous behavior where E / 0.0 produced infinity, | |
| // which saturated to u32::MAX when cast with `as u32`. | |
| return u32::MAX; | |
| } |
| } else { | ||
| value.wrapping_neg() |
There was a problem hiding this comment.
The abs_i64 function handles i64::MIN incorrectly. When value is i64::MIN, wrapping_neg() returns i64::MIN (because -i64::MIN overflows). This means abs_i64(i64::MIN) returns i64::MIN, which is still negative. This negative value is then added to total_weight at line 138, potentially causing incorrect total_weight calculations. Consider either documenting this limitation or using value.abs() with proper overflow handling, or saturating to i64::MAX.
| } else { | |
| value.wrapping_neg() | |
| } else if value == i64::MIN { | |
| i64::MAX | |
| } else { | |
| value.abs() |
|
Cool! Thanks. I'll take a look soon... |
2 participants
Related to #35