Ignore null buffer when creating ArrayData if null count is zero #2056
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2056 +/- ##
==========================================
+ Coverage 83.55% 83.57% +0.01%
==========================================
Files 222 222
Lines 58230 58244 +14
==========================================
+ Hits 48656 48679 +23
+ Misses 9574 9565 -9
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Why don't we use the BooleanBuilder directly? I find it use the same logic:
https://github.com/apache/arrow-rs/blob/master/arrow/src/array/builder/boolean_builder.rs#L128-L138
If we don't do further optimization, I guess we could write the code like this:
fn from_iter<I: IntoIterator<Item = Ptr>>(iter: I) -> Self {
let iter = iter.into_iter();
let (_, data_len) = iter.size_hint();
let data_len = data_len.expect("Iterator must be sized"); // panic if no upper bound.
let mut array_builder = BooleanBuilder::new(data_len);
iter.for_each(|i| {
array_builder.append_option(i.borrow().clone()).unwrap();
});
array_builder.finish()
}BTW, we could do some optimization in the BooleanBuilder, such as lazily materializing the null builder, just as what we have done in the primitive builder: https://github.com/apache/arrow-rs/blob/master/arrow/src/array/builder/primitive_builder.rs#L33-L35
|
Not sure what happened with the windows builder, but I have restarted the failed CI check and hopefully it will pass on rerun |
From a correctness perspective that would probably be better and would also solve the reliance on |
Do you mean that the Boolean builder is slower than this implementation? |
|
FWIW #2038 by @heyrutvik will overlap with this. This isn't a problem, just an FYI |
|
#2038 looks like the more extensive and correct solution. I'll take a look at the performance results for that PR. |
|
@jhorstmann FYI, see #2038 (comment) for some discussion about performance of the PR (and subsequent comments). I tried original value for buffer size for some builders but not seeing much difference. |
arrow/src/array/array_boolean.rs
Outdated
| @@ -242,14 +242,19 @@ impl<Ptr: Borrow<Option<bool>>> FromIterator<Ptr> for BooleanArray { | |||
| } | |||
| }); | |||
|
|
|||
| let null_buf: Buffer = null_builder.into(); | |||
There was a problem hiding this comment.
I wonder if we could push this optimisation into ArrayData::new_unchecked??
There was a problem hiding this comment.
That's an interesting idea and it passes all the existing test cases. It also changes behavior when someone passes an explicit null buffer, but I can't think of a reason why people would rely on that.
…er if there are no null values
jhorstmann
changed the title
|
Benchmark runs are scheduled for baseline = efd3152 and contender = b2cf02c. b2cf02c is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2055.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?
Semantically a null buffer with all bits set / a null count of 0 is the same as a None null buffer, but this is an observable behavior change.