refactor: expand signed zeros before selecting IN-list filters

Add the missing signed-zero encoding once in the shared static-filter
factory after dictionary flattening. Select the filter using the expanded
physical non-null list length.

Remove per-filter zero insertion and extra-length branchless kernels.
Cover both signs across cutoffs and preserve nulls, NaN payloads and slices.

Author: geoffreyclaude

datafusion/physical-expr/src/expressions/in_list/branchless_filter.rs

@@ -38,9 +38,8 @@
 //! `Float32` and a `UInt32` both use four bytes per value. The filter compares
 //! those stored bits through an unsigned type of the same size, without copying
 //! the value buffer. A bit pattern is simply the bytes Arrow uses to store a
-//! value. Float comparisons treat `0.0` and `-0.0` as equal under SQL
-//! semantics, while preserving distinctions between different NaN values.
-//! [`BranchlessFilterType`] defines these safe,
+//! value. Comparing it preserves details such as `0.0` versus `-0.0` and
+//! different NaN values. [`BranchlessFilterType`] defines these safe,
 //! same-sized mappings and checks their sizes at compile time.
 //!
 //! The fast path is limited to short lists:
@@ -76,7 +75,6 @@ use arrow::buffer::{BooleanBuffer, ScalarBuffer};
 use arrow::datatypes::*;
 use arrow::util::bit_iterator::BitIndexIterator;
 use datafusion_common::{Result, exec_datafusion_err, internal_datafusion_err};
-use half::f16;
 
 use super::result::build_result_from_contains;
 use super::static_filter::StaticFilter;
@@ -119,24 +117,20 @@ const BRANCHLESS_MAX_16B: usize = 4;
 ///
 /// `T` is the logical Arrow type accepted by the filter. `CompareType` is the
 /// same-width type used for the fixed comparison chain. Signed integers,
-/// floats, and temporal values use an unsigned comparison type. Most values
-/// compare by raw bit pattern; floats additionally treat both signed-zero
-/// patterns as equal.
+/// floats, and temporal values use an unsigned comparison type so they compare
+/// by their raw bit pattern.
 pub(super) trait BranchlessFilterType:
-    ArrowPrimitiveType + Send + Sync + Sized + 'static
+    ArrowPrimitiveType + Send + Sync + 'static
 {
     type CompareType: ArrowPrimitiveType + Send + Sync + 'static;
 
     /// Maximum number of non-null IN-list values to handle with
     /// [`BranchlessFilter`] for this primitive type.
     const MAX_LIST_LEN: usize;
-
-    /// The two signed-zero encodings for float types.
-    const SIGNED_ZERO_BITS: Option<[BranchlessNative<Self>; 2]> = None;
 }
 
 macro_rules! branchless_filter_type {
-    ($logical:ty, $compare:ty, $max_len:expr $(, $zero_bits:expr)?) => {
+    ($logical:ty, $compare:ty, $max_len:expr) => {
         // The branchless filter reads the same Arrow value buffer as the
         // comparison type. That is only valid when both native types have the
         // same width, so catch any bad mapping here at compile time.
@@ -149,7 +143,6 @@ macro_rules! branchless_filter_type {
         impl BranchlessFilterType for $logical {
             type CompareType = $compare;
             const MAX_LIST_LEN: usize = $max_len;
-            $(const SIGNED_ZERO_BITS: Option<[BranchlessNative<Self>; 2]> = Some($zero_bits);)?
         }
     };
 }
@@ -158,33 +151,18 @@ branchless_filter_type!(Int8Type, UInt8Type, BRANCHLESS_MAX_1B);
 branchless_filter_type!(UInt8Type, UInt8Type, BRANCHLESS_MAX_1B);
 branchless_filter_type!(Int16Type, UInt16Type, BRANCHLESS_MAX_2B);
 branchless_filter_type!(UInt16Type, UInt16Type, BRANCHLESS_MAX_2B);
-branchless_filter_type!(
-    Float16Type,
-    UInt16Type,
-    BRANCHLESS_MAX_2B,
-    [f16::ZERO.to_bits(), f16::NEG_ZERO.to_bits()]
-);
+branchless_filter_type!(Float16Type, UInt16Type, BRANCHLESS_MAX_2B);
 
 branchless_filter_type!(Int32Type, UInt32Type, BRANCHLESS_MAX_4B);
 branchless_filter_type!(UInt32Type, UInt32Type, BRANCHLESS_MAX_4B);
-branchless_filter_type!(
-    Float32Type,
-    UInt32Type,
-    BRANCHLESS_MAX_4B,
-    [0.0_f32.to_bits(), (-0.0_f32).to_bits()]
-);
+branchless_filter_type!(Float32Type, UInt32Type, BRANCHLESS_MAX_4B);
 branchless_filter_type!(Date32Type, UInt32Type, BRANCHLESS_MAX_4B);
 branchless_filter_type!(Time32SecondType, UInt32Type, BRANCHLESS_MAX_4B);
 branchless_filter_type!(Time32MillisecondType, UInt32Type, BRANCHLESS_MAX_4B);
 
 branchless_filter_type!(Int64Type, UInt64Type, BRANCHLESS_MAX_8B);
 branchless_filter_type!(UInt64Type, UInt64Type, BRANCHLESS_MAX_8B);
-branchless_filter_type!(
-    Float64Type,
-    UInt64Type,
-    BRANCHLESS_MAX_8B,
-    [0.0_f64.to_bits(), (-0.0_f64).to_bits()]
-);
+branchless_filter_type!(Float64Type, UInt64Type, BRANCHLESS_MAX_8B);
 branchless_filter_type!(Date64Type, UInt64Type, BRANCHLESS_MAX_8B);
 branchless_filter_type!(Time64MicrosecondType, UInt64Type, BRANCHLESS_MAX_8B);
 branchless_filter_type!(Time64NanosecondType, UInt64Type, BRANCHLESS_MAX_8B);
@@ -240,10 +218,8 @@ where
         }
 
         let all_values = branchless_values::<T>(in_array);
-        // Float zero may add its other signed encoding.
-        let mut in_list_values = Vec::with_capacity(
-            non_null_count + usize::from(T::SIGNED_ZERO_BITS.is_some()),
-        );
+        let mut in_list_values = Vec::with_capacity(non_null_count);
+
         match in_array.nulls() {
             None => {
                 in_list_values.extend(all_values.iter().copied());
@@ -258,18 +234,6 @@ where
         }
 
         debug_assert_eq!(in_list_values.len(), non_null_count);
-
-        // Add the other signed zero once so per-row lookup stays branch-free.
-        if let Some([positive_zero, negative_zero]) = T::SIGNED_ZERO_BITS {
-            match (
-                in_list_values.contains(&positive_zero),
-                in_list_values.contains(&negative_zero),
-            ) {
-                (true, false) => in_list_values.push(negative_zero),
-                (false, true) => in_list_values.push(positive_zero),
-                _ => {}
-            }
-        }
         let in_list_values = in_list_values.into_boxed_slice();
         let check_values = membership_check_for_len::<T>(in_list_values.len());
 
@@ -326,23 +290,13 @@ where
     T: BranchlessFilterType,
     BranchlessNative<T>: Copy + PartialEq,
 {
-    /// Selects a fixed-size comparison function for the enclosing `len` and `T`.
-    ///
-    /// Arguments must enumerate every length from zero through the logical limit.
-    /// Their count is therefore one past that limit: the extra length supported
-    /// for floats when adding the other signed-zero encoding.
-    ///
-    /// The function is selected once during filter construction.
     macro_rules! choose {
-        ($($n:literal),+ $(,)?) => {{
-            const EXTRA_LEN: usize = [$($n),+].len();
+        ($($n:literal),* $(,)?) => {
             match len {
-                $($n => check_values::<BranchlessNative<T>, $n>,)+
-                EXTRA_LEN if T::SIGNED_ZERO_BITS.is_some() =>
-                    check_values::<BranchlessNative<T>, EXTRA_LEN>,
+                $($n => check_values::<BranchlessNative<T>, $n>,)*
                 _ => unreachable!("list length exceeds the configured limit"),
             }
-        }};
+        };
     }
 
     // Avoid creating checks for lengths a type does not support.
@@ -497,11 +451,11 @@ mod tests {
 
         assert_eq!(
             filter.contains(&needles, false)?,
-            BooleanArray::from(vec![Some(true), Some(true), Some(true), None, None])
+            BooleanArray::from(vec![None, Some(true), Some(true), None, None])
         );
         assert_eq!(
             filter.contains(&needles, true)?,
-            BooleanArray::from(vec![Some(false), Some(false), Some(false), None, None])
+            BooleanArray::from(vec![None, Some(false), Some(false), None, None])
         );
 
         let wrong_type = UInt16Array::from(vec![Some(0x8000), Some(0x7e01)]);
@@ -512,26 +466,20 @@ mod tests {
     }
 
     #[test]
-    fn branchless_filter_floats_use_sql_zero_equality() -> Result<()> {
+    fn branchless_filter_floats_use_bit_equality() -> Result<()> {
         let nan_a = f32::from_bits(0x7fc0_0001);
         let nan_b = f32::from_bits(0x7fc0_0002);
         let haystack: ArrayRef =
-            Arc::new(Float32Array::from(vec![Some(0.0), Some(nan_a)]));
+            Arc::new(Float32Array::from(vec![Some(-0.0), Some(nan_a)]));
         let filter = BranchlessFilter::<Float32Type>::try_new(&haystack)?;
         let needles =
             Float32Array::from(vec![Some(0.0), Some(-0.0), Some(nan_a), Some(nan_b)]);
 
         assert_eq!(
             filter.contains(&needles, false)?,
-            BooleanArray::from(vec![Some(true), Some(true), Some(true), Some(false)])
+            BooleanArray::from(vec![Some(false), Some(true), Some(true), Some(false)])
         );
 
-        // A list containing both encodings is not expanded.
-        let zero_only: ArrayRef =
-            Arc::new(Float32Array::from(vec![Some(0.0), Some(-0.0)]));
-        let filter = BranchlessFilter::<Float32Type>::try_new(&zero_only)?;
-        assert_eq!(filter.in_list_values.len(), 2);
-
         let nan_a = f64::from_bits(0x7ff8_0000_0000_0001);
         let nan_b = f64::from_bits(0x7ff8_0000_0000_0002);
         let haystack: ArrayRef =
@@ -542,7 +490,7 @@ mod tests {
 
         assert_eq!(
             filter.contains(&needles, false)?,
-            BooleanArray::from(vec![Some(true), Some(true), Some(true), Some(false)])
+            BooleanArray::from(vec![Some(false), Some(true), Some(true), Some(false)])
         );
 
         Ok(())

datafusion/physical-expr/src/expressions/in_list/primitive_filter.rs

@@ -27,7 +27,6 @@ use arrow::array::{Array, ArrayRef, AsArray, BooleanArray};
 use arrow::datatypes::*;
 use arrow::util::bit_iterator::BitIndexIterator;
 use datafusion_common::{HashSet, Result, exec_datafusion_err};
-use half::f16;
 
 use super::branchless_filter::{BranchlessFilter, BranchlessFilterType};
 use super::result::build_in_list_result;
@@ -200,9 +199,6 @@ trait BitmapFilterType: ArrowPrimitiveType + Send + Sync + 'static {
 
     /// Returns the index in the bitmap to check for this value.
     fn index(value: Self::Native) -> usize;
-
-    /// Bitmap indices for the two signed-zero encodings of a float type.
-    const SIGNED_ZERO_INDICES: Option<[usize; 2]> = None;
 }
 
 /// `Int8` has 256 possible bit patterns, so four `u64` words cover the full domain.
@@ -258,11 +254,6 @@ impl BitmapFilterType for Float16Type {
     fn index(value: Self::Native) -> usize {
         value.to_bits() as usize
     }
-
-    const SIGNED_ZERO_INDICES: Option<[usize; 2]> = Some([
-        f16::ZERO.to_bits() as usize,
-        f16::NEG_ZERO.to_bits() as usize,
-    ]);
 }
 
 /// `IN` filter backed by one bit per possible value.
@@ -300,13 +291,6 @@ where
                 }
             }
         }
-        // Store both signed zeros so per-row lookup stays branch-free.
-        if let Some([positive_zero, negative_zero]) = T::SIGNED_ZERO_INDICES
-            && (bits.get_bit(positive_zero) || bits.get_bit(negative_zero))
-        {
-            bits.set_bit(positive_zero);
-            bits.set_bit(negative_zero);
-        }
         Ok(Self {
             null_count: prim_array.null_count(),
             bits,
@@ -348,13 +332,6 @@ where
     }
 }
 
-/// Hash keys and the two signed-zero encodings for float types.
-trait HashSetKey<V>: From<V> + Eq + Hash + Sized {
-    const SIGNED_ZERO_KEYS: Option<[Self; 2]> = None;
-}
-
-impl<T> HashSetKey<T> for T where T: Copy + Eq + Hash {}
-
 /// Wrapper for f32 that implements Hash and Eq using bit comparison.
 /// This treats NaN values as equal to each other when they have the same bit pattern.
 #[derive(Clone, Copy)]
@@ -380,10 +357,6 @@ impl From<f32> for OrderedFloat32 {
     }
 }
 
-impl HashSetKey<f32> for OrderedFloat32 {
-    const SIGNED_ZERO_KEYS: Option<[Self; 2]> = Some([Self(0.0), Self(-0.0)]);
-}
-
 /// Wrapper for f64 that implements Hash and Eq using bit comparison.
 /// This treats NaN values as equal to each other when they have the same bit pattern.
 #[derive(Clone, Copy)]
@@ -409,10 +382,6 @@ impl From<f64> for OrderedFloat64 {
     }
 }
 
-impl HashSetKey<f64> for OrderedFloat64 {
-    const SIGNED_ZERO_KEYS: Option<[Self; 2]> = Some([Self(0.0), Self(-0.0)]);
-}
-
 /// Hash-set membership for primitive types.
 ///
 /// `K` defaults to the Arrow type's native value. Floats use an ordered wrapper
@@ -430,7 +399,7 @@ impl<T, K> PrimitiveHashSetFilter<T, K>
 where
     T: ArrowPrimitiveType,
     T::Native: Copy,
-    K: HashSetKey<T::Native>,
+    K: From<T::Native> + Eq + Hash,
 {
     fn try_new(in_array: &ArrayRef) -> Result<Self> {
         let in_array = in_array.as_primitive_opt::<T>().ok_or_else(|| {
@@ -444,13 +413,6 @@ where
             values.insert(K::from(value));
         }
 
-        // Store both signed zeros so per-row lookup stays branch-free.
-        if let Some([positive_zero, negative_zero]) = K::SIGNED_ZERO_KEYS
-            && (values.contains(&positive_zero) || values.contains(&negative_zero))
-        {
-            values.insert(positive_zero);
-            values.insert(negative_zero);
-        }
         Ok(Self {
             null_count: in_array.null_count(),
             values,
@@ -497,8 +459,9 @@ mod tests {
 
     use arrow::array::{
         DictionaryArray, Float16Array, Float32Array, Float64Array, Int8Array, Int16Array,
-        PrimitiveArray, UInt8Array, UInt16Array, UInt32Array,
+        UInt8Array, UInt16Array, UInt32Array,
     };
+    use half::f16;
 
     use super::super::dictionary_filter::DictionaryFilter;
 
@@ -544,28 +507,6 @@ mod tests {
         Ok(())
     }
 
-    #[test]
-    fn branchless_float_zero_expansion_handles_max_list_len() -> Result<()> {
-        fn assert_routed_filter<T: BranchlessFilterType>(
-            negative_zero: T::Native,
-            positive_zero: T::Native,
-        ) -> Result<()> {
-            // The mirror zero is appended after the full logical list.
-            let haystack: ArrayRef = Arc::new(PrimitiveArray::<T>::from_value(
-                negative_zero,
-                T::MAX_LIST_LEN,
-            ));
-            let filter = instantiate_branchless_filter(&haystack)?
-                .expect("a full float list still uses a branchless filter");
-            let needles = PrimitiveArray::<T>::from_value(positive_zero, 1);
-            assert_contains(filter.as_ref(), &needles, vec![Some(true)])
-        }
-
-        assert_routed_filter::<Float16Type>(f16::NEG_ZERO, f16::ZERO)?;
-        assert_routed_filter::<Float32Type>(-0.0, 0.0)?;
-        assert_routed_filter::<Float64Type>(-0.0, 0.0)
-    }
-
     #[test]
     fn primitive_hash_filter_handles_float_keys() -> Result<()> {
         let nan32 = f32::NAN;
@@ -583,14 +524,14 @@ mod tests {
         assert_contains(
             &filter,
             &needles,
-            vec![Some(true), Some(true), Some(true), Some(false), None],
+            vec![Some(true), Some(false), Some(true), Some(false), None],
         )?;
 
         let nan64 = f64::NAN;
-        let haystack: ArrayRef = Arc::new(Float64Array::from(vec![-0.0, nan64]));
+        let haystack: ArrayRef = Arc::new(Float64Array::from(vec![1.0, nan64]));
         let filter =
             PrimitiveHashSetFilter::<Float64Type, OrderedFloat64>::try_new(&haystack)?;
-        let needles = Float64Array::from(vec![Some(0.0), Some(nan64), Some(2.0)]);
+        let needles = Float64Array::from(vec![Some(1.0), Some(nan64), Some(2.0)]);
         assert_contains(&filter, &needles, vec![Some(true), Some(true), Some(false)])
     }
 
@@ -719,8 +660,8 @@ mod tests {
         );
         let filter = BitmapFilter::<Float16Type>::try_new(&haystack)?;
         let needles = Float16Array::from(vec![
-            Some(f16::from_f32(-0.0)),
             Some(f16::from_f32(0.0)),
+            Some(f16::from_f32(-0.0)),
             Some(nan_a),
             Some(nan_b),
             None,