BooleanBufferBuilder::append_packed (#1038)#1039
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1039 +/- ##
==========================================
+ Coverage 82.31% 82.33% +0.02%
==========================================
Files 168 168
Lines 49031 49094 +63
==========================================
+ Hits 40359 40421 +62
- Misses 8672 8673 +1
Continue to review full report at Codecov.
|
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
👍
In case anyone is interested, the relevant code in IOx is https://github.com/influxdata/influxdb_iox/blob/07ba629e2b5066dbbad237812bb22764ef28a8fb/arrow_util/src/bitset.rs#L111-L155
|
Logic looks correct to me. There is code with a similar goal already in |
|
Added packed_append_range, also from IOx, as I needed it for #1037 @jhorstmann I will look into benchmarking the approaches and report back. I'll mark this as a draft until I find time for this |
|
#1052 is merged now |
arrow/src/util/mod.rs
Outdated
There was a problem hiding this comment.
This could be made pub mod and then we could use it in IOx but wasn't sure about this
There was a problem hiding this comment.
Let's leave it as pub(crate) until the next round of parquet finagling is complete and then make a separate decision (PR) about if we should make it public or not
arrow/src/array/transform/utils.rs
Outdated
There was a problem hiding this comment.
This is moved to arrow::util::bit_mask so that it can be used within arrow::array::builder
tustvold
force-pushed
the
boolean-buffer-append-packed
branch
from
c98aff0 to
818e6e3
Compare
| @@ -0,0 +1,193 @@ | |||
| // Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
Split out from arrow::array::transform::util
|
As per @jhorstmann 's great suggestion, this instead now lifts the already extant code up from |
|
|
||
| let mut b = BooleanBufferBuilder::new(4); | ||
| // Overallocate capacity | ||
| let mut b = BooleanBufferBuilder::new(8); |
There was a problem hiding this comment.
Additional test for #1051
Edit: I think this is actually just the diff being unhelpful - this code exists on master...
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
I reviewed the changes and they look good to me. Thanks for the reviews @jhorstmann and @Dandandan
Will merge this when CI is happy
| Extend, _MutableArrayData, | ||
| utils::{resize_for_bits, set_bits}, | ||
| }; | ||
| use crate::util::bit_mask::set_bits; |
There was a problem hiding this comment.
just to be explicit the bit_mask crate is not exposed publically
arrow/src/util/mod.rs
Outdated
There was a problem hiding this comment.
Let's leave it as pub(crate) until the next round of parquet finagling is complete and then make a separate decision (PR) about if we should make it public or not
5 participants
Which issue does this PR close?
Closes #1038.
Rationale for this change
See ticket
What changes are included in this PR?
This copies the code from IOx into arrow proper
Are there any user-facing changes?
BooleanBufferBuilder::append_packedis public