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.
|
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 |
| @@ -18,6 +18,7 @@ | |||
| #[cfg(feature = "test_utils")] | |||
| pub mod bench_util; | |||
| pub mod bit_chunk_iterator; | |||
| pub(crate) mod bit_mask; | |||
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
| @@ -33,49 +26,6 @@ pub(super) fn resize_for_bits(buffer: &mut MutableBuffer, len: usize) { | |||
| } | |||
| } | |||
|
|
|||
| /// sets all bits on `write_data` on the range `[offset_write..offset_write+len]` to be equal to the | |||
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 |
| @@ -2713,7 +2734,8 @@ mod tests { | |||
| let buffer = b.finish(); | |||
| assert_eq!(1, buffer.len()); | |||
|
|
|||
| 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...
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
| @@ -18,6 +18,7 @@ | |||
| #[cfg(feature = "test_utils")] | |||
| pub mod bench_util; | |||
| pub mod bit_chunk_iterator; | |||
| pub(crate) mod bit_mask; | |||
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
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