Copilot commented on code in PR #191:
URL: https://github.com/apache/datasketches-rust/pull/191#discussion_r3744460187
##########
datasketches/src/frequencies/sketch.rs:
##########
@@ -131,11 +131,18 @@ impl<T: Eq + Hash> FrequentItemsSketch<T> {
Self::with_lg_map_sizes(lg_max_map_size, LG_MIN_MAP_SIZE)
}
- /// Returns true if the sketch is empty.
+ /// Returns true if the sketch has no active items.
+ ///
+ /// A purge can remove all active items while retaining a non-zero total
weight and
+ /// maximum error. Use [`Self::total_weight`] to distinguish that state
from a virgin sketch.
pub fn is_empty(&self) -> bool {
self.hash_map.num_active() == 0
}
+ fn is_virgin(&self) -> bool {
+ self.stream_weight == 0
+ }
Review Comment:
`is_virgin()` is currently defined as `stream_weight == 0`. Because
`stream_weight` is a `u64` updated via `+=`, it can wrap to 0 on overflow in
release builds; that would incorrectly treat a non-virgin sketch (possibly with
active items) as virgin, collapsing serialization to the 8-byte empty image and
causing `merge()` to skip state. Consider including `offset` and `num_active`
in the virgin predicate so it reflects the actual "never updated" state and is
robust against `stream_weight` wraparound/corrupt inputs.
##########
datasketches/tests/serde_tests/frequencies.rs:
##########
@@ -104,14 +104,26 @@ fn test_empty_round_trip() {
#[test]
fn test_purged_to_empty_round_trip() {
// Saturating the map with count-1 items makes the purge median 1, which
- // removes every counter and leaves a non-trivial sketch empty.
+ // removes every counter while retaining stream and error state.
let mut sketch = FrequentItemsSketch::<i64>::new(32);
for i in 0..=(32 * 3 / 4) {
sketch.update(i);
}
assert!(sketch.is_empty());
- let restored =
FrequentItemsSketch::<i64>::deserialize(&sketch.serialize()).unwrap();
+ assert_eq!(sketch.num_active_items(), 0);
+ assert_eq!(sketch.total_weight(), 25);
+ assert_eq!(sketch.maximum_error(), 1);
+ assert_eq!(sketch.upper_bound(&1000), 1);
+
+ let bytes = sketch.serialize();
+ assert_eq!(bytes.len(), 4 * size_of::<u64>());
+ let restored = FrequentItemsSketch::<i64>::deserialize(&bytes).unwrap();
Review Comment:
This test file calls `size_of::<...>()` but does not import
`std::mem::size_of` (and it is also used earlier in the file). As written, this
won’t compile on stable Rust unless `size_of` happens to be in scope via some
nonstandard prelude; please either add a module-level `use std::mem::size_of;`
or fully-qualify the calls as `std::mem::size_of::<...>()` throughout the file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
