From c59d9f9edae407ba8aad9f91cf034ad2f1791248 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Wed, 26 Aug 2026 06:41:18 -0400 Subject: [PATCH 1/5] Use runtime dispatch instead of monomorphized FixedSizeBinaryFilter --- .../in_list/fixed_size_binary_filter.rs | 66 +++++++++---------- 1 file changed, 32 insertions(+), 34 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs b/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs index 421c02ea1f2d5..29b09e6ae79f8 100644 --- a/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs +++ b/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs @@ -38,7 +38,6 @@ //! Reinterpreting an aligned Arrow buffer is zero-copy. An unaligned buffer is //! copied into aligned primitive storage before filter construction or probing. -use std::marker::PhantomData; use std::mem::size_of; use std::sync::Arc; @@ -84,16 +83,12 @@ where } /// Adapts a primitive filter to concrete, same-width `FixedSizeBinary` arrays. -struct FixedSizeBinaryFilter { +struct FixedSizeBinaryFilter { data_type: DataType, inner: StaticFilterRef, - _marker: PhantomData, } -impl StaticFilter for FixedSizeBinaryFilter -where - T: ArrowPrimitiveType + Send + Sync + 'static, -{ +impl StaticFilter for FixedSizeBinaryFilter { fn null_count(&self) -> usize { self.inner.null_count() } @@ -114,27 +109,25 @@ where self.data_type ) })?; - let primitive = reinterpret_as_primitive::(array)?; - self.inner.contains(&primitive, negated) + let primitive = reinterpret(array)?; + self.inner.contains(primitive.as_ref(), negated) } } -fn instantiate_for_primitive(array: &FixedSizeBinaryArray) -> Result -where - T: ArrowPrimitiveType + Send + Sync + 'static, -{ - let primitive: ArrayRef = Arc::new(reinterpret_as_primitive::(array)?); - let inner = instantiate_primitive_filter(&primitive)?.ok_or_else(|| { - internal_datafusion_err!( - "FixedSizeBinary filter: no primitive filter for {}", - primitive.data_type() - ) - })?; - Ok(Arc::new(FixedSizeBinaryFilter:: { - data_type: array.data_type().clone(), - inner, - _marker: PhantomData, - })) +/// Reinterprets a supported-width array as its same-width primitive array. +fn reinterpret(array: &FixedSizeBinaryArray) -> Result { + Ok(match array.value_size() { + 1 => Arc::new(reinterpret_as_primitive::(array)?) as ArrayRef, + 2 => Arc::new(reinterpret_as_primitive::(array)?), + 4 => Arc::new(reinterpret_as_primitive::(array)?), + 8 => Arc::new(reinterpret_as_primitive::(array)?), + 16 => Arc::new(reinterpret_as_primitive::(array)?), + width => { + return Err(internal_datafusion_err!( + "FixedSizeBinary filter: unsupported width {width}" + )); + } + }) } /// Creates an optimized filter for supported concrete `FixedSizeBinary` arrays. @@ -144,19 +137,24 @@ pub(super) fn instantiate_fixed_size_binary_filter( let DataType::FixedSizeBinary(width) = in_array.data_type() else { return Ok(None); }; + if !matches!(width, 1 | 2 | 4 | 8 | 16) { + return Ok(None); + } let Some(array) = in_array.as_fixed_size_binary_opt() else { return Ok(None); }; - let filter = match width { - 1 => instantiate_for_primitive::(array)?, - 2 => instantiate_for_primitive::(array)?, - 4 => instantiate_for_primitive::(array)?, - 8 => instantiate_for_primitive::(array)?, - 16 => instantiate_for_primitive::(array)?, - _ => return Ok(None), - }; - Ok(Some(filter)) + let primitive = reinterpret(array)?; + let inner = instantiate_primitive_filter(&primitive)?.ok_or_else(|| { + internal_datafusion_err!( + "FixedSizeBinary filter: no primitive filter for {}", + primitive.data_type() + ) + })?; + Ok(Some(Arc::new(FixedSizeBinaryFilter { + data_type: in_array.data_type().clone(), + inner, + }))) } #[cfg(test)] From ff5a973bf2d60ca3bd361faaff4852f566ffeee8 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Sun, 30 Aug 2026 08:22:53 -0400 Subject: [PATCH 2/5] Apply batched suggestions from code review Co-authored-by: Jay Zhan --- .../in_list/fixed_size_binary_filter.rs | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs b/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs index 29b09e6ae79f8..ddbc18ea839a4 100644 --- a/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs +++ b/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs @@ -109,25 +109,29 @@ impl StaticFilter for FixedSizeBinaryFilter { self.data_type ) })?; - let primitive = reinterpret(array)?; + let primitive = reinterpret(array)?.ok_or_else(|| { + internal_datafusion_err!( + "FixedSizeBinary filter: unsupported width {}", + array.value_size() + ) + })?; self.inner.contains(primitive.as_ref(), negated) } } -/// Reinterprets a supported-width array as its same-width primitive array. -fn reinterpret(array: &FixedSizeBinaryArray) -> Result { - Ok(match array.value_size() { - 1 => Arc::new(reinterpret_as_primitive::(array)?) as ArrayRef, - 2 => Arc::new(reinterpret_as_primitive::(array)?), - 4 => Arc::new(reinterpret_as_primitive::(array)?), - 8 => Arc::new(reinterpret_as_primitive::(array)?), - 16 => Arc::new(reinterpret_as_primitive::(array)?), - width => { - return Err(internal_datafusion_err!( - "FixedSizeBinary filter: unsupported width {width}" - )); - } - }) +/// Reinterprets a supported-width array as its same-width primitive array, or +/// `None` if the width has no primitive representation. +fn reinterpret(array: &FixedSizeBinaryArray) -> Result> { + Ok(Some(match array.value_size() { + 1 => Arc::new(reinterpret_as_primitive::(array)?) as ArrayRef, + 2 => Arc::new(reinterpret_as_primitive::(array)?), + 4 => Arc::new(reinterpret_as_primitive::(array)?), + 8 => Arc::new(reinterpret_as_primitive::(array)?), + 16 => Arc::new(reinterpret_as_primitive::(array)?), + + _ => return Ok(None), + })) + } } /// Creates an optimized filter for supported concrete `FixedSizeBinary` arrays. From b24af55dfe343aa88bfce33f69359f9734e94e98 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Sun, 30 Aug 2026 08:26:34 -0400 Subject: [PATCH 3/5] Apply batched suggestions from code review Co-authored-by: Jay Zhan --- .../in_list/fixed_size_binary_filter.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs b/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs index ddbc18ea839a4..f2a60883af341 100644 --- a/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs +++ b/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs @@ -148,7 +148,20 @@ pub(super) fn instantiate_fixed_size_binary_filter( return Ok(None); }; - let primitive = reinterpret(array)?; + /// Creates an optimized filter for supported concrete `FixedSizeBinary` arrays. + pub(super) fn instantiate_fixed_size_binary_filter( + in_array: &ArrayRef, + ) -> Result> { + if !matches!(in_array.data_type(), DataType::FixedSizeBinary(_)) { + return Ok(None); + } + let Some(array) = in_array.as_fixed_size_binary_opt() else { + return Ok(None); + }; + + let Some(primitive) = reinterpret(array)? else { + return Ok(None); + }; let inner = instantiate_primitive_filter(&primitive)?.ok_or_else(|| { internal_datafusion_err!( "FixedSizeBinary filter: no primitive filter for {}", From 3fd2602288d218f7099111bd04aa452ebf45224e Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Sun, 30 Aug 2026 08:27:57 -0400 Subject: [PATCH 4/5] Resolve conflicts --- .../in_list/fixed_size_binary_filter.rs | 37 ++++++------------- 1 file changed, 11 insertions(+), 26 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs b/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs index ffbbbace3a8e1..5f130d59064bb 100644 --- a/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs +++ b/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs @@ -108,11 +108,11 @@ impl StaticFilter for FixedSizeBinaryFilter { ) })?; let primitive = reinterpret(array)?.ok_or_else(|| { - internal_datafusion_err!( - "FixedSizeBinary filter: unsupported width {}", - array.value_size() - ) - })?; + internal_datafusion_err!( + "FixedSizeBinary filter: unsupported width {}", + array.value_size() + ) + })?; self.inner.contains(primitive.as_ref(), negated) } } @@ -121,42 +121,27 @@ impl StaticFilter for FixedSizeBinaryFilter { /// `None` if the width has no primitive representation. fn reinterpret(array: &FixedSizeBinaryArray) -> Result> { Ok(Some(match array.value_size() { - 1 => Arc::new(reinterpret_as_primitive::(array)?) as ArrayRef, - 2 => Arc::new(reinterpret_as_primitive::(array)?), - 4 => Arc::new(reinterpret_as_primitive::(array)?), - 8 => Arc::new(reinterpret_as_primitive::(array)?), - 16 => Arc::new(reinterpret_as_primitive::(array)?), + 1 => Arc::new(reinterpret_as_primitive::(array)?) as ArrayRef, + 2 => Arc::new(reinterpret_as_primitive::(array)?), + 4 => Arc::new(reinterpret_as_primitive::(array)?), + 8 => Arc::new(reinterpret_as_primitive::(array)?), + 16 => Arc::new(reinterpret_as_primitive::(array)?), _ => return Ok(None), })) - } } /// Creates an optimized filter for supported concrete `FixedSizeBinary` arrays. pub(super) fn instantiate_fixed_size_binary_filter( in_array: &ArrayRef, ) -> Result> { - let DataType::FixedSizeBinary(width) = in_array.data_type() else { - return Ok(None); - }; - if !matches!(width, 1 | 2 | 4 | 8 | 16) { + if !matches!(in_array.data_type(), DataType::FixedSizeBinary(_)) { return Ok(None); } let Some(array) = in_array.as_fixed_size_binary_opt() else { return Ok(None); }; - /// Creates an optimized filter for supported concrete `FixedSizeBinary` arrays. - pub(super) fn instantiate_fixed_size_binary_filter( - in_array: &ArrayRef, - ) -> Result> { - if !matches!(in_array.data_type(), DataType::FixedSizeBinary(_)) { - return Ok(None); - } - let Some(array) = in_array.as_fixed_size_binary_opt() else { - return Ok(None); - }; - let Some(primitive) = reinterpret(array)? else { return Ok(None); }; From de5bb6ebf8d7695e2065fc4e36d4833e09203059 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Sun, 30 Aug 2026 08:29:24 -0400 Subject: [PATCH 5/5] Add explanatory comments --- .../src/expressions/in_list/fixed_size_binary_filter.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs b/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs index 5f130d59064bb..b0e86cf932370 100644 --- a/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs +++ b/datafusion/physical-expr/src/expressions/in_list/fixed_size_binary_filter.rs @@ -108,6 +108,7 @@ impl StaticFilter for FixedSizeBinaryFilter { ) })?; let primitive = reinterpret(array)?.ok_or_else(|| { + // Instantiation code rejects unsupported widths, so this is unexpected internal_datafusion_err!( "FixedSizeBinary filter: unsupported width {}", array.value_size() @@ -146,6 +147,7 @@ pub(super) fn instantiate_fixed_size_binary_filter( return Ok(None); }; let inner = instantiate_primitive_filter(&primitive)?.ok_or_else(|| { + // reinterpret should have returned None for unsupported widths, so this is unexpected internal_datafusion_err!( "FixedSizeBinary filter: no primitive filter for {}", primitive.data_type()