Getting rid of the gcd dependency and using NonZeroU64 in gcd. (#1459)

quickwit-oss · Aug 22, 2022 · 67d94f5 · 67d94f5
1 parent abbd934
commit 67d94f5
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 19 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -60,7 +60,6 @@ pretty_assertions = "1.2.1"
 serde_cbor = { version = "0.11.2", optional = true }
 async-trait = "0.1.53"
 arc-swap = "1.5.0"
-gcd = "2.1.0"
 
 [target.'cfg(windows)'.dependencies]
 winapi = "0.3.9"

diff --git a/src/fastfield/gcd.rs b/src/fastfield/gcd.rs
@@ -1,9 +1,9 @@
 use std::io::{self, Write};
+use std::num::NonZeroU64;
 
 use common::BinarySerializable;
 use fastdivide::DividerU64;
 use fastfield_codecs::FastFieldCodecReader;
-use gcd::Gcd;
 use ownedbytes::OwnedBytes;
 
 pub const GCD_DEFAULT: u64 = 1;
@@ -56,38 +56,54 @@ pub fn write_gcd_header<W: Write>(field_write: &mut W, min_value: u64, gcd: u64)
     Ok(())
 }
 
+/// Compute the gcd of two non null numbers.
+///
+/// It is recommended, but not required, to feed values such that `large >= small`.
+fn compute_gcd(mut large: NonZeroU64, mut small: NonZeroU64) -> NonZeroU64 {
+    loop {
+        let rem: u64 = large.get() % small;
+        if let Some(new_small) = NonZeroU64::new(rem) {
+            (large, small) = (small, new_small);
+        } else {
+            return small;
+        }
+    }
+}
+
 // Find GCD for iterator of numbers
-pub fn find_gcd(numbers: impl Iterator<Item = u64>) -> Option<u64> {
-    let mut numbers = numbers.filter(|n| *n != 0);
-    let mut gcd = numbers.next()?;
-    if gcd == 1 {
-        return Some(1);
+pub fn find_gcd(numbers: impl Iterator<Item = u64>) -> Option<NonZeroU64> {
+    let mut numbers = numbers.flat_map(NonZeroU64::new);
+    let mut gcd: NonZeroU64 = numbers.next()?;
+    if gcd.get() == 1 {
+        return Some(gcd);
     }
 
-    let mut gcd_divider = DividerU64::divide_by(gcd);
+    let mut gcd_divider = DividerU64::divide_by(gcd.get());
     for val in numbers {
-        let remainder = val - (gcd_divider.divide(val)) * gcd;
+        let remainder = val.get() - (gcd_divider.divide(val.get())) * gcd.get();
         if remainder == 0 {
             continue;
         }
-        gcd = gcd.gcd(val);
-        if gcd == 1 {
-            return Some(1);
+        gcd = compute_gcd(val, gcd);
+        if gcd.get() == 1 {
+            return Some(gcd);
         }
 
-        gcd_divider = DividerU64::divide_by(gcd);
+        gcd_divider = DividerU64::divide_by(gcd.get());
     }
     Some(gcd)
 }
 
 #[cfg(test)]
 mod tests {
     use std::collections::HashMap;
+    use std::num::NonZeroU64;
     use std::path::Path;
 
     use common::HasLen;
 
     use crate::directory::{CompositeFile, RamDirectory, WritePtr};
+    use crate::fastfield::gcd::compute_gcd;
     use crate::fastfield::serializer::FastFieldCodecEnableCheck;
     use crate::fastfield::tests::{FIELD, FIELDI64, SCHEMA, SCHEMAI64};
     use crate::fastfield::{
@@ -212,14 +228,30 @@ mod tests {
         assert_eq!(test_fastfield.get(2), 300);
     }
 
+    #[test]
+    fn test_compute_gcd() {
+        let test_compute_gcd_aux = |large, small, expected| {
+            let large = NonZeroU64::new(large).unwrap();
+            let small = NonZeroU64::new(small).unwrap();
+            let expected = NonZeroU64::new(expected).unwrap();
+            assert_eq!(compute_gcd(small, large), expected);
+            assert_eq!(compute_gcd(large, small), expected);
+        };
+        test_compute_gcd_aux(1, 4, 1);
+        test_compute_gcd_aux(2, 4, 2);
+        test_compute_gcd_aux(10, 25, 5);
+        test_compute_gcd_aux(25, 25, 25);
+    }
+
     #[test]
     fn find_gcd_test() {
         assert_eq!(find_gcd([0].into_iter()), None);
-        assert_eq!(find_gcd([0, 10].into_iter()), Some(10));
-        assert_eq!(find_gcd([10, 0].into_iter()), Some(10));
+        assert_eq!(find_gcd([0, 10].into_iter()), NonZeroU64::new(10));
+        assert_eq!(find_gcd([10, 0].into_iter()), NonZeroU64::new(10));
         assert_eq!(find_gcd([].into_iter()), None);
-        assert_eq!(find_gcd([15, 30, 5, 10].into_iter()), Some(5));
-        assert_eq!(find_gcd([15, 16, 10].into_iter()), Some(1));
-        assert_eq!(find_gcd([0, 5, 5, 5].into_iter()), Some(5));
+        assert_eq!(find_gcd([15, 30, 5, 10].into_iter()), NonZeroU64::new(5));
+        assert_eq!(find_gcd([15, 16, 10].into_iter()), NonZeroU64::new(1));
+        assert_eq!(find_gcd([0, 5, 5, 5].into_iter()), NonZeroU64::new(5));
+        assert_eq!(find_gcd([0, 0].into_iter()), None);
     }
 }
diff --git a/src/fastfield/serializer/mod.rs b/src/fastfield/serializer/mod.rs
@@ -1,4 +1,5 @@
 use std::io::{self, Write};
+use std::num::NonZeroU64;
 
 use common::{BinarySerializable, CountingWriter};
 pub use fastfield_codecs::bitpacked::{
@@ -140,7 +141,9 @@ impl CompositeFastFieldSerializer {
         I: Iterator<Item = u64>,
     {
         let field_write = self.composite_write.for_field_with_idx(field, idx);
-        let gcd = find_gcd(iter_gen().map(|val| val - stats.min_value)).unwrap_or(GCD_DEFAULT);
+        let gcd = find_gcd(iter_gen().map(|val| val - stats.min_value))
+            .map(NonZeroU64::get)
+            .unwrap_or(GCD_DEFAULT);
 
         if gcd == 1 {
             return Self::create_auto_detect_u64_fast_field_with_idx_gcd(