-
Notifications
You must be signed in to change notification settings - Fork 750
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
Ignore null buffer when creating ArrayData if null count is zero #2056
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could push this optimisation into ArrayData::new_unchecked??
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.
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
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.