fix: clarify and vs and_kleene in stream.rs and filtering.rs (#1289)

In both `RowFilter` and `stream.rs`, their effect is irrelevant because we later convert nulls into falses.
spiraldb · Nov 13, 2024 · 1c5a4a3 · 1c5a4a3
1 parent 1249a7d
commit 1c5a4a3
Show file tree

Hide file tree

Showing 2 changed files with 11 additions and 5 deletions.
diff --git a/vortex-serde/src/file/read/filtering.rs b/vortex-serde/src/file/read/filtering.rs
@@ -5,7 +5,7 @@ use std::sync::Arc;
 use arrow_buffer::BooleanBuffer;
 use vortex_array::aliases::hash_set::HashSet;
 use vortex_array::array::{BoolArray, ConstantArray};
-use vortex_array::compute::and;
+use vortex_array::compute::and_kleene;
 use vortex_array::stats::ArrayStatistics;
 use vortex_array::validity::Validity;
 use vortex_array::{Array, IntoArray, IntoArrayVariant};
@@ -62,7 +62,9 @@ impl VortexExpr for RowFilter {
             }
 
             let new_mask = expr.evaluate(batch)?;
-            mask = and(new_mask, mask)?;
+            // Either `and` or `and_kleene` is fine. They only differ on `false AND null`, but
+            // null_as_false only cares which values are true.
+            mask = and_kleene(new_mask, mask)?;
         }
 
         null_as_false(mask.into_bool()?)

diff --git a/vortex-serde/src/file/read/stream.rs b/vortex-serde/src/file/read/stream.rs
@@ -10,7 +10,7 @@ use futures_util::future::BoxFuture;
 use futures_util::{stream, FutureExt, StreamExt, TryStreamExt};
 use itertools::Itertools;
 use vortex_array::array::ChunkedArray;
-use vortex_array::compute::and;
+use vortex_array::compute::and_kleene;
 use vortex_array::stats::ArrayStatistics;
 use vortex_array::Array;
 use vortex_dtype::DType;
@@ -224,8 +224,12 @@ impl<R: VortexReadAt + Unpin + 'static> VortexFileArrayStream<R> {
                     }
                     Some(BatchRead::Batch(mut batch)) => {
                         if let Some(row_mask) = &self.row_mask {
-                            batch =
-                                and(batch, row_mask.slice(sel_begin, sel_end).to_mask_array()?)?;
+                            // Either `and` or `and_kleene` is fine. They only differ on `false AND
+                            // null`, but RowMask::from_mask_array only cares which values are true.
+                            batch = and_kleene(
+                                batch,
+                                row_mask.slice(sel_begin, sel_end).to_mask_array()?,
+                            )?;
                         }
 
                         if batch