Merge pull request #42 from marshallpierce/deser-dont-set-min-value-for-zero-count

Fix occasional test failures caused by incorrect min value tracking

Author: jonhoo

src/lib.rs

@@ -173,7 +173,7 @@ const ORIGINAL_MAX: u64 = 0;
 /// of percentiles.
 pub trait Counter
     : num::Num + num::ToPrimitive + num::FromPrimitive + num::Saturating + num::CheckedSub
-    + num::CheckedAdd + Copy + PartialOrd<Self>{
+    + num::CheckedAdd + Copy + PartialOrd<Self> {
 }
 
 // auto-implement marker trait

src/serialization/deserializer.rs

@@ -240,12 +240,11 @@ pub fn zig_zag_decode(encoded: u64) -> i64 {
 /// We need to perform the same logic in two different decode loops while carrying over a modicum
 /// of state.
 struct DecodeLoopState<T: Counter> {
-    dest_index:usize,
+    dest_index: usize,
     phantom: PhantomData<T>
 }
 
-impl <T: Counter> DecodeLoopState<T> {
-
+impl<T: Counter> DecodeLoopState<T> {
     fn new() -> DecodeLoopState<T> {
         DecodeLoopState {
             dest_index: 0,
@@ -266,10 +265,12 @@ impl <T: Counter> DecodeLoopState<T> {
             let count: T = T::from_i64(count_or_zeros)
                 .ok_or(DeserializeError::UnsuitableCounterType)?;
 
-            h.set_count_at_index(self.dest_index, count)
-                .map_err(|_| DeserializeError::EncodedArrayTooLong)?;
+            if count > T::zero() {
+                h.set_count_at_index(self.dest_index, count)
+                    .map_err(|_| DeserializeError::EncodedArrayTooLong)?;
 
-            restat_state.on_nonzero_count(self.dest_index, count);
+                restat_state.on_nonzero_count(self.dest_index, count);
+            }
 
             self.dest_index = self.dest_index.checked_add(1)
                 .ok_or(DeserializeError::UsizeTypeTooSmall)?;

src/serialization/tests.rs

@@ -515,16 +515,15 @@ fn do_serialize_roundtrip_random<T>(max_count: T)
     let mut s = V2Serializer::new();
     let mut d = Deserializer::new();
     let mut vec = Vec::new();
-    let mut rng = rand::weak_rng();
+    let mut count_rng = rand::weak_rng();
 
     let range = Range::<T>::new(T::one(), max_count);
     for _ in 0..100 {
         vec.clear();
         let mut h = Histogram::<T>::new_with_bounds(1, u64::max_value(), 3).unwrap();
 
-        for _ in 0..1000 {
-            let count = range.ind_sample(&mut rng);
-            let value = rng.gen();
+        for value in RandomVarintEncodedLengthIter::new(rand::weak_rng()).take(1000) {
+            let count = range.ind_sample(&mut count_rng);
             // don't let accumulated per-value count exceed max_count
             let existing_count = h.count_at(value).unwrap();
             let sum = existing_count.saturating_add(count);
@@ -633,3 +632,45 @@ impl<T: SampleRange, R: Rng> Iterator for RandomRangeIter<T, R> {
         Some(self.range.ind_sample(&mut self.rng))
     }
 }
+
+// Evenly distributed random numbers end up biased heavily towards longer encoded byte lengths:
+// there are a lot more large numbers than there are small (duh), but for exercising serialization
+// code paths, we'd like many at all byte lengths. This is also arguably more representative of
+// real data. This should emit values whose varint lengths are uniformly distributed across the
+// whole length range (1 to 9).
+struct RandomVarintEncodedLengthIter<R: Rng> {
+    ranges: [Range<u64>; 9],
+    range_for_picking_range: Range<usize>,
+    rng: R
+}
+
+impl<R: Rng> RandomVarintEncodedLengthIter<R> {
+    fn new(rng: R) -> RandomVarintEncodedLengthIter<R> {
+        RandomVarintEncodedLengthIter {
+            ranges: [
+                Range::new(smallest_number_in_n_byte_varint(1), largest_number_in_n_byte_varint(1) + 1),
+                Range::new(smallest_number_in_n_byte_varint(2), largest_number_in_n_byte_varint(2) + 1),
+                Range::new(smallest_number_in_n_byte_varint(3), largest_number_in_n_byte_varint(3) + 1),
+                Range::new(smallest_number_in_n_byte_varint(4), largest_number_in_n_byte_varint(4) + 1),
+                Range::new(smallest_number_in_n_byte_varint(5), largest_number_in_n_byte_varint(5) + 1),
+                Range::new(smallest_number_in_n_byte_varint(6), largest_number_in_n_byte_varint(6) + 1),
+                Range::new(smallest_number_in_n_byte_varint(7), largest_number_in_n_byte_varint(7) + 1),
+                Range::new(smallest_number_in_n_byte_varint(8), largest_number_in_n_byte_varint(8) + 1),
+                Range::new(smallest_number_in_n_byte_varint(9), largest_number_in_n_byte_varint(9)),
+            ],
+            range_for_picking_range: Range::new(0, 9),
+            rng: rng
+        }
+    }
+}
+
+impl<R: Rng> Iterator for RandomVarintEncodedLengthIter<R> {
+    type Item = u64;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        // pick the range we'll use
+        let value_range = self.ranges[self.range_for_picking_range.ind_sample(&mut self.rng)];
+
+        Some(value_range.ind_sample(&mut self.rng))
+    }
+}

src/serialization/v2_serializer.rs

@@ -107,7 +107,8 @@ pub fn encode_counts<T: Counter>(h: &Histogram<T>, buf: &mut [u8]) -> Result<usi
         let count = unsafe { *(h.counts.get_unchecked(index)) };
         index += 1;
 
-        // Non-negative values are positive counts or 1 zero, negative values are repeated zeros.
+        // Non-negative values are counts for the respective value, negative values are skipping
+        // that many (absolute value) zero-count values.
 
         let mut zero_count = 0;
         if count == T::zero() {