From 0311e22668464b7f01e1a3e7aa8697dffc5186e3 Mon Sep 17 00:00:00 2001
From: Jorge Leitao <jorgecarleitao@gmail.com>
Date: Thu, 30 Dec 2021 06:04:05 +0000
Subject: [PATCH] Fixed error in compare booleans (#721)

---
 src/compute/comparison/boolean.rs | 170 +---------------------------
 tests/it/compute/comparison.rs    | 177 +++++++++++++++++++++++++++++-
 2 files changed, 180 insertions(+), 167 deletions(-)

diff --git a/src/compute/comparison/boolean.rs b/src/compute/comparison/boolean.rs
index db35fe90767..2b7df893017 100644
--- a/src/compute/comparison/boolean.rs
+++ b/src/compute/comparison/boolean.rs
@@ -27,7 +27,7 @@ pub fn compare_op_scalar<F>(lhs: &BooleanArray, rhs: bool, op: F) -> BooleanArra
 where
     F: Fn(u64, u64) -> u64,
 {
-    let rhs = if rhs { 0b11111111 } else { 0 };
+    let rhs = if rhs { !0 } else { 0 };
 
     let values = unary(lhs.values(), |x| op(x, rhs));
     BooleanArray::from_data(DataType::Boolean, values, lhs.validity().cloned())
@@ -84,7 +84,8 @@ pub fn lt_eq(lhs: &BooleanArray, rhs: &BooleanArray) -> BooleanArray {
 /// Null values are less than non-null values.
 pub fn lt_eq_scalar(lhs: &BooleanArray, rhs: bool) -> BooleanArray {
     if rhs {
-        compare_op_scalar(lhs, rhs, |_, _| 0b11111111)
+        let all_ones = !0;
+        compare_op_scalar(lhs, rhs, |_, _| all_ones)
     } else {
         compare_op_scalar(lhs, rhs, |a, _| !a)
     }
@@ -122,168 +123,7 @@ pub fn gt_eq_scalar(lhs: &BooleanArray, rhs: bool) -> BooleanArray {
     if rhs {
         lhs.clone()
     } else {
-        compare_op_scalar(lhs, rhs, |_, _| 0b11111111)
-    }
-}
-
-// disable wrapping inside literal vectors used for test data and assertions
-#[rustfmt::skip::macros(vec)]
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    macro_rules! cmp_bool {
-        ($KERNEL:ident, $A_VEC:expr, $B_VEC:expr, $EXPECTED:expr) => {
-            let a = BooleanArray::from_slice($A_VEC);
-            let b = BooleanArray::from_slice($B_VEC);
-            let c = $KERNEL(&a, &b);
-            assert_eq!(BooleanArray::from_slice($EXPECTED), c);
-        };
-    }
-
-    macro_rules! cmp_bool_options {
-        ($KERNEL:ident, $A_VEC:expr, $B_VEC:expr, $EXPECTED:expr) => {
-            let a = BooleanArray::from($A_VEC);
-            let b = BooleanArray::from($B_VEC);
-            let c = $KERNEL(&a, &b);
-            assert_eq!(BooleanArray::from($EXPECTED), c);
-        };
-    }
-
-    macro_rules! cmp_bool_scalar {
-        ($KERNEL:ident, $A_VEC:expr, $B:literal, $EXPECTED:expr) => {
-            let a = BooleanArray::from_slice($A_VEC);
-            let c = $KERNEL(&a, $B);
-            assert_eq!(BooleanArray::from_slice($EXPECTED), c);
-        };
-    }
-
-    #[test]
-    fn test_eq() {
-        cmp_bool!(
-            eq,
-            &[true, false, true, false],
-            &[true, true, false, false],
-            &[true, false, false, true]
-        );
-    }
-
-    #[test]
-    fn test_eq_scalar() {
-        cmp_bool_scalar!(eq_scalar, &[false, true], true, &[false, true]);
-    }
-
-    #[test]
-    fn test_eq_with_slice() {
-        let a = BooleanArray::from_slice(&[true, true, false]);
-        let b = BooleanArray::from_slice(&[true, true, true, true, false]);
-        let c = b.slice(2, 3);
-        let d = eq(&c, &a);
-        assert_eq!(d, BooleanArray::from_slice(&[true, true, true]));
-    }
-
-    #[test]
-    fn test_neq() {
-        cmp_bool!(
-            neq,
-            &[true, false, true, false],
-            &[true, true, false, false],
-            &[false, true, true, false]
-        );
-    }
-
-    #[test]
-    fn test_neq_scalar() {
-        cmp_bool_scalar!(neq_scalar, &[false, true], true, &[true, false]);
-    }
-
-    #[test]
-    fn test_lt() {
-        cmp_bool!(
-            lt,
-            &[true, false, true, false],
-            &[true, true, false, false],
-            &[false, true, false, false]
-        );
-    }
-
-    #[test]
-    fn test_lt_scalar_true() {
-        cmp_bool_scalar!(lt_scalar, &[false, true], true, &[true, false]);
-    }
-
-    #[test]
-    fn test_lt_scalar_false() {
-        cmp_bool_scalar!(lt_scalar, &[false, true], false, &[false, false]);
-    }
-
-    #[test]
-    fn test_lt_eq_scalar_true() {
-        cmp_bool_scalar!(lt_eq_scalar, &[false, true], true, &[true, true]);
-    }
-
-    #[test]
-    fn test_lt_eq_scalar_false() {
-        cmp_bool_scalar!(lt_eq_scalar, &[false, true], false, &[true, false]);
-    }
-
-    #[test]
-    fn test_gt_scalar_true() {
-        cmp_bool_scalar!(gt_scalar, &[false, true], true, &[false, false]);
-    }
-
-    #[test]
-    fn test_gt_scalar_false() {
-        cmp_bool_scalar!(gt_scalar, &[false, true], false, &[false, true]);
-    }
-
-    #[test]
-    fn test_gt_eq_scalar_true() {
-        cmp_bool_scalar!(gt_eq_scalar, &[false, true], true, &[false, true]);
-    }
-
-    #[test]
-    fn test_gt_eq_scalar_false() {
-        cmp_bool_scalar!(gt_eq_scalar, &[false, true], false, &[true, true]);
-    }
-
-    #[test]
-    fn eq_nulls() {
-        cmp_bool_options!(
-            eq,
-            &[
-                None,
-                None,
-                None,
-                Some(false),
-                Some(false),
-                Some(false),
-                Some(true),
-                Some(true),
-                Some(true)
-            ],
-            &[
-                None,
-                Some(false),
-                Some(true),
-                None,
-                Some(false),
-                Some(true),
-                None,
-                Some(false),
-                Some(true)
-            ],
-            &[
-                None,
-                None,
-                None,
-                None,
-                Some(true),
-                Some(false),
-                None,
-                Some(false),
-                Some(true)
-            ]
-        );
+        let all_ones = !0;
+        compare_op_scalar(lhs, rhs, |_, _| all_ones)
     }
 }
diff --git a/tests/it/compute/comparison.rs b/tests/it/compute/comparison.rs
index 4dbdff5fd24..0fdb3e35532 100644
--- a/tests/it/compute/comparison.rs
+++ b/tests/it/compute/comparison.rs
@@ -1,11 +1,12 @@
-use arrow2::array::new_null_array;
-use arrow2::compute::comparison::{can_eq, eq, eq_scalar};
+use arrow2::array::*;
+use arrow2::compute::comparison::boolean::*;
 use arrow2::datatypes::DataType::*;
 use arrow2::datatypes::TimeUnit;
 use arrow2::scalar::new_scalar;
 
 #[test]
 fn consistency() {
+    use arrow2::compute::comparison::*;
     let datatypes = vec![
         Null,
         Boolean,
@@ -56,3 +57,175 @@ fn consistency() {
         }
     });
 }
+
+// disable wrapping inside literal vectors used for test data and assertions
+#[rustfmt::skip::macros(vec)]
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    macro_rules! cmp_bool {
+        ($KERNEL:ident, $A_VEC:expr, $B_VEC:expr, $EXPECTED:expr) => {
+            let a = BooleanArray::from_slice($A_VEC);
+            let b = BooleanArray::from_slice($B_VEC);
+            let c = $KERNEL(&a, &b);
+            assert_eq!(BooleanArray::from_slice($EXPECTED), c);
+        };
+    }
+
+    macro_rules! cmp_bool_options {
+        ($KERNEL:ident, $A_VEC:expr, $B_VEC:expr, $EXPECTED:expr) => {
+            let a = BooleanArray::from($A_VEC);
+            let b = BooleanArray::from($B_VEC);
+            let c = $KERNEL(&a, &b);
+            assert_eq!(BooleanArray::from($EXPECTED), c);
+        };
+    }
+
+    macro_rules! cmp_bool_scalar {
+        ($KERNEL:ident, $A_VEC:expr, $B:literal, $EXPECTED:expr) => {
+            let a = BooleanArray::from_slice($A_VEC);
+            let c = $KERNEL(&a, $B);
+            assert_eq!(BooleanArray::from_slice($EXPECTED), c);
+        };
+    }
+
+    #[test]
+    fn test_eq() {
+        cmp_bool!(
+            eq,
+            &[true, false, true, false],
+            &[true, true, false, false],
+            &[true, false, false, true]
+        );
+    }
+
+    #[test]
+    fn test_eq_scalar() {
+        cmp_bool_scalar!(eq_scalar, &[false, true], true, &[false, true]);
+    }
+
+    #[test]
+    fn test_eq_with_slice() {
+        let a = BooleanArray::from_slice(&[true, true, false]);
+        let b = BooleanArray::from_slice(&[true, true, true, true, false]);
+        let c = b.slice(2, 3);
+        let d = eq(&c, &a);
+        assert_eq!(d, BooleanArray::from_slice(&[true, true, true]));
+    }
+
+    #[test]
+    fn test_neq() {
+        cmp_bool!(
+            neq,
+            &[true, false, true, false],
+            &[true, true, false, false],
+            &[false, true, true, false]
+        );
+    }
+
+    #[test]
+    fn test_neq_scalar() {
+        cmp_bool_scalar!(neq_scalar, &[false, true], true, &[true, false]);
+    }
+
+    #[test]
+    fn test_lt() {
+        cmp_bool!(
+            lt,
+            &[true, false, true, false],
+            &[true, true, false, false],
+            &[false, true, false, false]
+        );
+    }
+
+    #[test]
+    fn test_lt_scalar_true() {
+        cmp_bool_scalar!(lt_scalar, &[false, true], true, &[true, false]);
+    }
+
+    #[test]
+    fn test_lt_scalar_false() {
+        cmp_bool_scalar!(lt_scalar, &[false, true], false, &[false, false]);
+    }
+
+    #[test]
+    fn test_lt_eq_scalar_true() {
+        cmp_bool_scalar!(lt_eq_scalar, &[false, true], true, &[true, true]);
+    }
+
+    #[test]
+    fn test_lt_eq_scalar_false() {
+        cmp_bool_scalar!(lt_eq_scalar, &[false, true], false, &[true, false]);
+    }
+
+    #[test]
+    fn test_gt_scalar_true() {
+        cmp_bool_scalar!(gt_scalar, &[false, true], true, &[false, false]);
+    }
+
+    #[test]
+    fn test_gt_scalar_false() {
+        cmp_bool_scalar!(gt_scalar, &[false, true], false, &[false, true]);
+    }
+
+    #[test]
+    fn test_gt_eq_scalar_true() {
+        cmp_bool_scalar!(gt_eq_scalar, &[false, true], true, &[false, true]);
+    }
+
+    #[test]
+    fn test_gt_eq_scalar_false() {
+        cmp_bool_scalar!(gt_eq_scalar, &[false, true], false, &[true, true]);
+    }
+
+    #[test]
+    fn test_lt_eq_scalar_true_1() {
+        cmp_bool_scalar!(
+            lt_eq_scalar,
+            &[false, true, true, true, true, true, true, true, false],
+            true,
+            &[true, true, true, true, true, true, true, true, true]
+        );
+    }
+
+    #[test]
+    fn eq_nulls() {
+        cmp_bool_options!(
+            eq,
+            &[
+                None,
+                None,
+                None,
+                Some(false),
+                Some(false),
+                Some(false),
+                Some(true),
+                Some(true),
+                Some(true)
+            ],
+            &[
+                None,
+                Some(false),
+                Some(true),
+                None,
+                Some(false),
+                Some(true),
+                None,
+                Some(false),
+                Some(true)
+            ],
+            &[
+                None,
+                None,
+                None,
+                None,
+                Some(true),
+                Some(false),
+                None,
+                Some(false),
+                Some(true)
+            ]
+        );
+    }
+}