-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Optimize Int8 and Int16 integer IN filters #23299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,23 +78,60 @@ impl BitmapStorage for Box<[u64; 1024]> { | |
| /// Arrow primitive types supported by [`BitmapFilter`]. | ||
| /// | ||
| /// Arrow already defines the Rust value type as `T::Native`. This trait only | ||
| /// supplies the bitmap storage size for the two integer domains that are small | ||
| /// enough to represent with one bit per possible value. | ||
| /// supplies the bitmap storage size and maps values to their bit-pattern index | ||
| /// for the two integer domains that are small enough to represent with one bit | ||
| /// per possible value. | ||
| pub(super) trait BitmapFilterType: | ||
| ArrowPrimitiveType + Send + Sync + 'static | ||
| { | ||
| type Storage: BitmapStorage; | ||
|
|
||
| /// Returns the index in the bitmap to check for this value. | ||
| fn index(value: Self::Native) -> usize; | ||
| } | ||
|
|
||
| /// `Int8` has 256 possible bit patterns, so four `u64` words cover the full domain. | ||
| impl BitmapFilterType for Int8Type { | ||
| type Storage = [u64; 4]; | ||
|
|
||
| #[inline(always)] | ||
| fn index(value: Self::Native) -> usize { | ||
| // Reinterpret the signed value's bit pattern into a bitmap index. | ||
| value as u8 as usize | ||
| } | ||
| } | ||
|
|
||
| /// `UInt8` has 256 possible values, so four `u64` words cover the full domain. | ||
| impl BitmapFilterType for UInt8Type { | ||
| type Storage = [u64; 4]; | ||
|
|
||
| #[inline(always)] | ||
| fn index(value: Self::Native) -> usize { | ||
| value as usize | ||
| } | ||
| } | ||
|
|
||
| /// `Int16` has 65,536 possible bit patterns, so 1,024 `u64` words cover the full | ||
| /// domain. | ||
| impl BitmapFilterType for Int16Type { | ||
| type Storage = Box<[u64; 1024]>; | ||
|
|
||
| #[inline(always)] | ||
| fn index(value: Self::Native) -> usize { | ||
| // Reinterpret the signed value's bit pattern into a bitmap index. | ||
| value as u16 as usize | ||
| } | ||
| } | ||
|
|
||
| /// `UInt16` has 65,536 possible values, so 1,024 `u64` words cover the full | ||
| /// domain. | ||
| impl BitmapFilterType for UInt16Type { | ||
| type Storage = Box<[u64; 1024]>; | ||
|
|
||
| #[inline(always)] | ||
| fn index(value: Self::Native) -> usize { | ||
| value as usize | ||
| } | ||
| } | ||
|
|
||
| /// `IN` filter backed by one bit per possible value. | ||
|
|
@@ -121,14 +158,14 @@ where | |
| match prim_array.nulls() { | ||
| None => { | ||
| for &v in values { | ||
| bits.set_bit(v.as_usize()); | ||
| bits.set_bit(T::index(v)); | ||
| } | ||
| } | ||
| Some(nulls) => { | ||
| for i in | ||
| BitIndexIterator::new(nulls.validity(), nulls.offset(), nulls.len()) | ||
| { | ||
| bits.set_bit(values[i].as_usize()); | ||
| bits.set_bit(T::index(values[i])); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -140,7 +177,7 @@ where | |
|
|
||
| #[inline(always)] | ||
| fn check(&self, needle: T::Native) -> bool { | ||
| self.bits.get_bit(needle.as_usize()) | ||
| self.bits.get_bit(T::index(needle)) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -359,9 +396,6 @@ macro_rules! primitive_static_filter { | |
| }; | ||
| } | ||
|
|
||
| // Generate specialized filters for all integer primitive types | ||
| primitive_static_filter!(Int8StaticFilter, Int8Type); | ||
| primitive_static_filter!(Int16StaticFilter, Int16Type); | ||
| primitive_static_filter!(Int32StaticFilter, Int32Type); | ||
| primitive_static_filter!(Int64StaticFilter, Int64Type); | ||
| primitive_static_filter!(UInt32StaticFilter, UInt32Type); | ||
|
|
@@ -384,7 +418,7 @@ mod tests { | |
| use super::*; | ||
| use std::sync::Arc; | ||
|
|
||
| use arrow::array::{DictionaryArray, Int8Array, UInt8Array, UInt16Array}; | ||
| use arrow::array::{DictionaryArray, Int8Array, Int16Array, UInt8Array, UInt16Array}; | ||
|
|
||
| fn assert_contains( | ||
| filter: &dyn StaticFilter, | ||
|
|
@@ -425,6 +459,28 @@ mod tests { | |
| assert_contains(&filter, &needles, vec![Some(true), None, None, Some(true)]) | ||
| } | ||
|
|
||
| #[test] | ||
| fn bitmap_filter_i8_handles_signed_boundaries_and_slices() -> Result<()> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One small follow-up idea: it could be useful to add a public-path regression through
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good idea -- I reviewed the existing slt (SQL) test coverage and it doesn't really have comprensive IN list coverage -- I will add some in a follow on PR
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tracking with an issue: First PR |
||
| let haystack: ArrayRef = Arc::new( | ||
| Int8Array::from(vec![Some(99), Some(i8::MIN), None, Some(-1), Some(42)]) | ||
| .slice(1, 3), | ||
| ); | ||
| let filter = BitmapFilter::<Int8Type>::try_new(&haystack)?; | ||
| let needles = | ||
| Int8Array::from(vec![Some(7), Some(i8::MIN), Some(-1), None]).slice(1, 3); | ||
|
|
||
| assert_eq!( | ||
| filter.contains(&needles, false)?, | ||
| BooleanArray::from(vec![Some(true), Some(true), None]) | ||
| ); | ||
| assert_eq!( | ||
| filter.contains(&needles, true)?, | ||
| BooleanArray::from(vec![Some(false), Some(false), None]) | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn bitmap_filter_u16_handles_boundaries_and_nulls() -> Result<()> { | ||
| let haystack: ArrayRef = Arc::new(UInt16Array::from(vec![ | ||
|
|
@@ -449,4 +505,33 @@ mod tests { | |
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn bitmap_filter_i16_handles_signed_boundaries_and_slices() -> Result<()> { | ||
| let haystack: ArrayRef = Arc::new( | ||
| Int16Array::from(vec![ | ||
| Some(123), | ||
| Some(i16::MIN), | ||
| None, | ||
| Some(-1), | ||
| Some(i16::MAX), | ||
| ]) | ||
| .slice(1, 4), | ||
| ); | ||
| let filter = BitmapFilter::<Int16Type>::try_new(&haystack)?; | ||
| let needles = | ||
| Int16Array::from(vec![Some(0), Some(i16::MIN), Some(7), Some(i16::MAX)]) | ||
| .slice(1, 3); | ||
|
|
||
| assert_eq!( | ||
| filter.contains(&needles, false)?, | ||
| BooleanArray::from(vec![Some(true), None, Some(true)]) | ||
| ); | ||
| assert_eq!( | ||
| filter.contains(&needles, true)?, | ||
| BooleanArray::from(vec![Some(false), None, Some(false)]) | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea is just to reinterpret the signed value into a usize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good as an comment within the code
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea -- done in 90bf7cc