[GitHub] [arrow-rs] alamb commented on a change in pull request #296: fix invalid null handling in filter

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

alamb commented on a change in pull request #296:
URL: https://github.com/apache/arrow-rs/pull/296#discussion_r634773487



##########
File path: arrow/src/compute/kernels/filter.rs
##########
@@ -581,4 +602,27 @@ mod tests {
         assert_eq!(chunks, vec![(1, 62), (63, 124), (125, 130)]);
         assert_eq!(filter_count, 61 + 61 + 5);
     }
+
+    #[test]
+    fn test_null_mask() -> Result<()> {
+        use crate::compute::kernels::comparison;
+        let a: PrimitiveArray<Int64Type> =
+            PrimitiveArray::from(vec![Some(1), Some(2), None]);
+        let mask0 = comparison::eq(&a, &a)?;
+        let out0 = filter(&a, &mask0)?;
+        let out_arr0 = out0
+            .as_any()
+            .downcast_ref::<PrimitiveArray<Int64Type>>()
+            .unwrap();
+
+        let mask1 = BooleanArray::from(vec![Some(true), Some(true), None]);
+        let out1 = filter(&a, &mask1)?;
+        let out_arr1 = out1
+            .as_any()
+            .downcast_ref::<PrimitiveArray<Int64Type>>()
+            .unwrap();
+        assert_eq!(mask0, mask1);
+        assert_eq!(out_arr0, out_arr1);

Review comment:
       This check and test makes sense to me (that the result of filtering using the output of `eq` should equal the output of filtering with a boolean mask that has nulls) 👍 
   
   I also ran this test with the change in this PR commented out and it failed (as expected) in this way:
   
   ```
   ---- compute::kernels::filter::tests::test_null_mask stdout ----
   thread 'compute::kernels::filter::tests::test_null_mask' panicked at 'assertion failed: `(left == right)`
     left: `PrimitiveArray<Int64>
   [
     1,
     2,
     null,
   ]`,
    right: `PrimitiveArray<Int64>
   [
     1,
     2,
   ]`', arrow/src/compute/kernels/filter.rs:606:9
   note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
   ```
   
   So 👍 




-- 
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.

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