[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1163: Support `=`, `<`, `<=`, `>`, `>=`, `!=`, `is distinct from`, `is not distinct from` for `BooleanArray`

[GitBox <gi...@apache.org>]

alamb commented on a change in pull request #1163:
URL: https://github.com/apache/arrow-datafusion/pull/1163#discussion_r749708025



##########
File path: datafusion/src/physical_optimizer/pruning.rs
##########
@@ -1374,36 +1374,24 @@ mod tests {
 
     #[test]
     fn prune_bool_column_eq_true() {
-        let (schema, statistics, _, _) = bool_setup();
+        let (schema, statistics, expected_true, _) = bool_setup();
 
         // b1 = true
         let expr = col("b1").eq(lit(true));
         let p = PruningPredicate::try_new(&expr, schema).unwrap();
-        let result = p.prune(&statistics).unwrap_err();
-        assert!(
-            result.to_string().contains(
-                "Data type Boolean not supported for scalar operation 'lt_eq' on dyn array"
-            ),
-            "{}",
-            result
-        )
+        let result = p.prune(&statistics).unwrap();
+        assert_eq!(result, expected_true);

Review comment:
       pruning works for boolean columns now

##########
File path: datafusion/src/physical_plan/expressions/binary.rs
##########
@@ -49,6 +53,62 @@ use super::coercion::{
     eq_coercion, like_coercion, numerical_coercion, order_coercion, string_coercion,
 };
 
+// Simple (low performance) kernels until optimized kernels are added to arrow
+// TODO: file arrow-rs ticket to track this feature
+
+fn is_distinct_from_bool(
+    left: &BooleanArray,
+    right: &BooleanArray,
+) -> Result<BooleanArray> {
+    // Different from `eq` beacause not eq_bool because null == null
+    Ok(left
+        .iter()
+        .zip(right.iter())
+        .map(|(left, right)| Some(left != right))
+        .collect())
+}
+
+fn is_not_distinct_from_bool(
+    left: &BooleanArray,
+    right: &BooleanArray,
+) -> Result<BooleanArray> {
+    Ok(left
+        .iter()
+        .zip(right.iter())
+        .map(|(left, right)| Some(left == right))
+        .collect())
+}
+
+#[allow(clippy::bool_comparison)]

Review comment:
       Clippy said `left < right` was clearer if expressed as `!left && right` which I happen to disagree with in this case, so I ignored the clippy lint

##########
File path: datafusion/src/physical_plan/expressions/binary.rs
##########
@@ -49,6 +53,62 @@ use super::coercion::{
     eq_coercion, like_coercion, numerical_coercion, order_coercion, string_coercion,
 };
 
+// Simple (low performance) kernels until optimized kernels are added to arrow
+// TODO: file arrow-rs ticket to track this feature

Review comment:
       Question to reviewers: Are these useful kernels to have in arrow? Or are they so special purpose we should leave them just in DataFusion?

##########
File path: datafusion/src/physical_plan/expressions/binary.rs
##########
@@ -276,6 +377,7 @@ macro_rules! binary_array_op_scalar {
             DataType::Date64 => {
                 compute_op_scalar!($LEFT, $RIGHT, $OP, Date64Array)
             }
+            DataType::Boolean => compute_bool_op_scalar!($LEFT, $RIGHT, $OP, BooleanArray),

Review comment:
       adding this line and the one below it adds all the new support, which is kind of cool! It is terrifying how many functions end up being called :)

##########
File path: datafusion/src/physical_plan/file_format/parquet.rs
##########
@@ -714,12 +714,14 @@ mod tests {
     }
 
     #[test]
-    fn row_group_predicate_builder_unsupported_type() -> Result<()> {
+    fn row_group_predicate_builder_null_expr() -> Result<()> {
         use crate::logical_plan::{col, lit};
-        // test row group predicate with unsupported statistics type (boolean)
-        // where a null array is generated for some statistics columns
-        // int > 1 and bool = true => c1_max > 1 and null
-        let expr = col("c1").gt(lit(15)).and(col("c2").eq(lit(true)));
+        // test row group predicate with an unknown (Null) expr

Review comment:
       now bool stats don't result in null columns, so I needed to use a constant to get the same effect




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org