Use NullBuffer in ArrayData (#3775) #3778
Conversation
github-actions
bot
added
arrow
| None => { | ||
| let len_bytes = ceil(len, 8); | ||
| MutableBuffer::new(len_bytes) | ||
| .with_bitset(len_bytes, true) | ||
| .into() | ||
| } | ||
| Some(buffer) => buffer.bit_slice(input.offset(), len), | ||
| Some(nulls) => nulls.inner().sliced(), |
There was a problem hiding this comment.
Eventually the use of a sliceable NullBuffer will allow us to not need to slice the buffer, and instead just preserve the offset, but I wanted to keep the changes in this PR down
| @@ -481,9 +481,10 @@ mod tests { | |||
| assert_eq!(batches.len(), 1); | |||
|
|
|||
| let list = as_list_array(batches[0].column(0).as_ref()); | |||
| assert_eq!(list.len(), 3); | |||
There was a problem hiding this comment.
This test was wrong, it was checking nulls beyond the bounds of the NullBuffer. This was working because previously the length enforcement was at the byte level (i.e. multiple of 8)
|
apache/datafusion#5441 shows this causing extremely limited downstream churn to DataFusion 🎉 Edit: I have also confirmed this does not regress the benchmarks |
| @@ -249,23 +249,15 @@ impl<K: ArrowPrimitiveType> DictionaryArray<K> { | |||
|
|
|||
| // Note: This use the ArrayDataBuilder::build_unchecked and afterwards | |||
| // call the new function which only validates that the keys are in bounds. | |||
| let mut data = ArrayData::builder(dict_data_type) | |||
There was a problem hiding this comment.
This was actually previously wrong, as it doesn't preserve the offset of the input array
arrow-buffer/src/buffer/boolean.rs
Outdated
| pub fn slice(&self, offset: usize, len: usize) -> Self { | ||
| assert!( | ||
| offset.saturating_add(len) <= self.len, | ||
| "the offset of the new BooleanBuffer cannot exceed the existing length" |
There was a problem hiding this comment.
| "the offset of the new BooleanBuffer cannot exceed the existing length" | |
| "the offset + length of the new BooleanBuffer cannot exceed the existing length" |
arrow-buffer/src/buffer/null.rs
Outdated
| @@ -69,11 +86,28 @@ impl NullBuffer { | |||
| !self.is_valid(idx) | |||
| } | |||
|
|
|||
| /// Returns the inner buffer | |||
| /// Returns the packed validity of this [`BooleanBuffer`] not including any offset | |||
There was a problem hiding this comment.
| /// Returns the packed validity of this [`BooleanBuffer`] not including any offset | |
| /// Returns the packed validity of inner [`BooleanBuffer`] of this [`NullBuffer`] not including any offset |
| @@ -1704,6 +1710,7 @@ pub struct ArrayDataBuilder { | |||
| len: usize, | |||
| null_count: Option<usize>, | |||
There was a problem hiding this comment.
Do we still need null_count in this builder?
There was a problem hiding this comment.
Yes, so that we can support use-cases that provide null_bit_buffer instead of NullBuffer
|
Benchmark runs are scheduled for baseline = 7852e76 and contender = eff058f. eff058f 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 #3775
Part of #1799
Part of #1176
Rationale for this change
See ticket, but we need to switch to a null buffer representation that can store its offset separately from the ArrayData it belongs to. There isn't an obvious way to add support for this without a breaking change. Fortunately most downstreams are not interacting with the null buffers directly, and so the impact of this change shouldn't be too problematic.
What changes are included in this PR?
Are there any user-facing changes?