Jefffrey commented on code in PR #11257:
URL: https://github.com/apache/arrow-rs/pull/11257#discussion_r4242247743
##########
arrow-select/src/filter.rs:
##########
@@ -1693,6 +1711,67 @@ mod tests {
assert_eq!(9, d.value(1));
}
+ // Sliced at offset 1 so the null bits sit off a byte boundary, with a
+ // partial trailing word. Rows 0 and 129 are null.
+ fn nullable_slice(null_every: i32) -> Int32Array {
Review Comment:
the numbers chosen here seem odd and out of place; why choose a big value
like 131? and why are rows 0 and 129 guaranteed to be null here, wouldnt that
depend on the input?
##########
arrow-select/src/filter.rs:
##########
@@ -2750,4 +2829,13 @@ mod tests {
let filter = FilterBuilder::new(&predicate).build();
filter_native(&values, &filter);
}
+
+ #[test]
+ #[should_panic(expected = "nulls.len() >= len")]
+ fn test_filter_nulls_shorter_than_filter() {
+ let nulls = NullBuffer::from_iter((0..8).map(|i| i != 1));
+ let predicate = BooleanArray::from_iter((0..9).map(|i| Some(i == 0)));
Review Comment:
instead of constructing these from a mapped iter we can just construct them
directly from vecs of bools
##########
arrow-select/src/filter.rs:
##########
@@ -1693,6 +1711,67 @@ mod tests {
assert_eq!(9, d.value(1));
}
+ // Sliced at offset 1 so the null bits sit off a byte boundary, with a
+ // partial trailing word. Rows 0 and 129 are null.
+ fn nullable_slice(null_every: i32) -> Int32Array {
+ let values: Int32Array = (0..131)
+ .map(|i| (i % null_every != 1).then_some(i))
+ .collect();
+ values.slice(1, 130)
+ }
+
+ fn filter_masks(keep: impl Fn(usize) -> bool) -> [BooleanArray; 2] {
+ let mask: BooleanArray = (0..130).map(|i| Some(keep(i))).collect();
+ let sliced: BooleanArray = (0..131).map(|i| Some(i > 0 && keep(i -
1))).collect();
+ [mask, sliced.slice(1, 130)]
+ }
+
+ #[test]
+ fn test_filter_nulls_selecting_only_valid_rows() {
+ // Every 43rd row null puts the dense filter above the slices threshold
+ for null_every in [3, 43] {
+ let values = nullable_slice(null_every);
+ let nulls = values.nulls().unwrap();
+ let masks = filter_masks(|i| nulls.is_valid(i))
+ .into_iter()
+ .chain(filter_masks(|i| i == 128));
Review Comment:
why is this chain necessary, what does it achieve in the test?
##########
arrow-select/src/filter.rs:
##########
@@ -1693,6 +1711,67 @@ mod tests {
assert_eq!(9, d.value(1));
}
+ // Sliced at offset 1 so the null bits sit off a byte boundary, with a
+ // partial trailing word. Rows 0 and 129 are null.
+ fn nullable_slice(null_every: i32) -> Int32Array {
+ let values: Int32Array = (0..131)
+ .map(|i| (i % null_every != 1).then_some(i))
+ .collect();
+ values.slice(1, 130)
+ }
+
+ fn filter_masks(keep: impl Fn(usize) -> bool) -> [BooleanArray; 2] {
+ let mask: BooleanArray = (0..130).map(|i| Some(keep(i))).collect();
+ let sliced: BooleanArray = (0..131).map(|i| Some(i > 0 && keep(i -
1))).collect();
+ [mask, sliced.slice(1, 130)]
+ }
+
+ #[test]
+ fn test_filter_nulls_selecting_only_valid_rows() {
+ // Every 43rd row null puts the dense filter above the slices threshold
Review Comment:
which slices threshold are we talking about here?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
