Accept any &dyn Array in nullif kernel#2940
Conversation
| assert_eq!(r, vec![None, Some("a"), None]); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
These tests are borrowed from @bjchambers original PR #521
| ) -> Buffer | ||
| where | ||
| F: Fn(u64, u64) -> u64, | ||
| F: FnMut(u64, u64) -> u64, |
There was a problem hiding this comment.
This is just a drive by fix to make this accept mutable closures - this is a backwards compatible change
|
cc @jhorstmann |
|
Didn't examine it, but looks a lot simpler than the previous attempts, so that's nice! Curious if something changed to make this easier? |
I don't think anything changed, but rather than offsetting values as with previous attempts, it just pads the null buffer. This is simpler and highly unlikely to have a meaningful performance or memory impact, as zero-allocating a buffer is effectively free, and null masks are very compressed |
arrow/src/compute/kernels/boolean.rs
Outdated
| @@ -470,27 +473,19 @@ pub fn is_not_null(input: &dyn Array) -> Result<BooleanArray> { | |||
|
|
|||
| /// Copies original array, setting null bit to true if a secondary comparison boolean array is set to true. | |||
There was a problem hiding this comment.
Not related to the change, but I got confused while reading this.
Based on following comment:
// left=1 right=1 & comp=true output bitmap=null
// left=1 right=1 & comp=false output bitmap=set
I think that if the boolean array is set to true, null bit will be set to false?
There was a problem hiding this comment.
I think this is confusion caused by the fact the null masks, are actually validity masks... Will clarify docs
| let t = !b; | ||
| valid_count += t.count_ones() as usize; | ||
| t | ||
| }), |
There was a problem hiding this comment.
The logic looks the same as before.
|
Benchmark runs are scheduled for baseline = 99e205f and contender = 3a90654. 3a90654 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
5 participants
Which issue does this PR close?
Closes #510
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?