tisonkun commented on code in PR #191:
URL: https://github.com/apache/datasketches-rust/pull/191#discussion_r3747113240
##########
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:
Good catch. I replaced `is_virgin()` with `is_initial_state()` and now
require `stream_weight`, `offset`, and the active-item count all to be zero. I
also added regression coverage for zero stream weight with retained active
items or offset, checking both serialization and merge.
##########
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:
`size_of` is available from the standard prelude since Rust 1.80
(`size_of_prelude`). This workspace has MSRV 1.86.0, and both the local MSRV
build and the existing CI compile this test successfully without an explicit
import, so no change is needed here. Reference:
https://doc.rust-lang.org/stable/src/std/prelude/v1.rs.html#21-27
--
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]
