Flesh out NullBuffer abstraction (#3880) #3885

tustvold · 2023-03-17T17:47:46Z

Which issue does this PR close?

Part of #3880

Rationale for this change

This continues the process of moving to more expressive buffer abstractions

What changes are included in this PR?

Are there any user-facing changes?

tustvold · 2023-03-17T17:48:18Z

arrow-arith/src/arity.rs

 use arrow_schema::ArrowError;
 use std::sync::Arc;

 #[inline]
 unsafe fn build_primitive_array<O: ArrowPrimitiveType>(
    len: usize,
    buffer: Buffer,
-    null_count: usize,
-    null_buffer: Option<Buffer>,
+    nulls: Option<NullBuffer>,


The astute will notice this is precisely the PrimitiveArray::new_unchecked that #3880 proposes to add 🎉

tustvold · 2023-03-17T17:49:10Z

arrow-string/src/regexp.rs

-            vec![result.finish()],
-            vec![],
-        )
+        ArrayDataBuilder::new(DataType::Boolean)


A future BooleanArray::new_unchecked should clean this up 😄

tustvold · 2023-03-17T17:49:25Z

arrow-data/src/lib.rs

@@ -23,7 +23,7 @@ pub use data::*;
 mod equal;
 pub mod transform;

-pub mod bit_iterator;
+pub use arrow_buffer::bit_iterator;


This is moved to arrow_buffer

…ties

tustvold · 2023-03-17T18:03:05Z

arrow-arith/src/arity.rs

-        .as_ref()
-        .map(|x| len - x.count_set_bits_offset(0, len))
-        .unwrap_or_default();
+    let nulls = NullBuffer::union(a.nulls(), b.nulls());


Not only is this less code, but it can now preserve the source null buffers even if they have an offset

alamb

Makes sense to me -- the code looks much nicer to me

Please run benchmarks prior to merging this -- while it shouldn't make any difference I can imagine silly things like cross crate inlining or whatever might cause unexpected performance slowdown

cc @viirya

alamb · 2023-03-20T19:32:03Z

arrow-arith/src/arity.rs

@@ -391,17 +372,12 @@ where
    if a.null_count() == 0 && b.null_count() == 0 {
        try_binary_no_nulls_mut(len, a, b, op)
    } else {
-        let null_buffer = combine_option_bitmap(&[a.data(), b.data()], len);
-        let null_count = null_buffer


it is nice that the null count now is encapsulated in NullBuffer rather than needing to be calculated explicitly (and potentially inconsistently)

alamb · 2023-03-20T19:33:05Z

arrow-arith/src/arity.rs

-            .null_count(null_count);
-
-        let array_data = unsafe { array_builder.build_unchecked() };
+        let array_builder = builder.finish().into_data().into_builder();


I wonder why we need to make a new builder (rather than just update the one we had) -- though I see this is what the previous code did too

PrimitiveBuilder vs ArrayBuilder, this should be greatly simplified with the ongoing work in #3880

alamb · 2023-03-20T19:34:01Z

arrow-buffer/src/buffer/boolean.rs

@@ -145,3 +146,41 @@ impl BooleanBuffer {
        self.buffer
    }
 }
+
+impl BitAnd<&BooleanBuffer> for &BooleanBuffer {


This is very nice 👍 -- it will be really nice to apply such bit operations on BooleanBuffer / NullBuffers directly and not via some function

alamb · 2023-03-20T19:36:47Z

arrow-buffer/src/buffer/boolean.rs

+    fn bitand(self, rhs: &BooleanBuffer) -> Self::Output {
+        assert_eq!(self.len, rhs.len);
+        BooleanBuffer {
+            buffer: buffer_bin_and(


I noticed above that buffers are is combined using combine_option_bitmap which is different than this.

However, when I double checked that combine_option_bitmap calls eventually to buffer_bin_and

arrow-rs/arrow-data/src/bit_mask.rs

Lines 71 to 98 in e41e0ca

pub fn combine_option_bitmap(

arrays: &[&ArrayData],

len_in_bits: usize,

) -> Option<Buffer> {

let (buffer, offset) = arrays

.iter()

.map(|array| match array.nulls() {

Some(n) => (Some(n.buffer().clone()), n.offset()),

None => (None, 0),

})

.reduce(|acc, buffer_and_offset| match (acc, buffer_and_offset) {

((None, _), (None, _)) => (None, 0),

((Some(buffer), offset), (None, _)) | ((None, _), (Some(buffer), offset)) => {

(Some(buffer), offset)

}

((Some(buffer_left), offset_left), (Some(buffer_right), offset_right)) => (

Some(buffer_bin_and(

&buffer_left,

offset_left,

&buffer_right,

offset_right,

len_in_bits,

)),

0,

),

})?;

Some(buffer?.bit_slice(offset, len_in_bits))

viirya

Looks good to me. It looks much simple and clear.

viirya · 2023-03-20T22:25:39Z

arrow-buffer/src/buffer/null.rs

@@ -50,6 +51,18 @@ impl NullBuffer {
        Self { buffer, null_count }
    }

+    /// Computes the union of two optional [`NullBuffer`]
+    pub fn union(


This is bit and, but I think union sounds like bit or? Maybe bitand?

Its union of the nulls, its confusing because a 0 bit is a null... I'll add further docs

tustvold · 2023-03-21T12:21:04Z

I've confirmed this to have no major impact on benchmarks

* Flesh out NullBuffer abstraction (apache#3880) * Review feedback

Flesh out NullBuffer abstraction (apache#3880)

66ff479

github-actions bot added the arrow Changes to the arrow crate label Mar 17, 2023

tustvold commented Mar 17, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/master' into more-buffer-utili…

35821d7

…ties

github-actions bot added the parquet Changes to the parquet crate label Mar 17, 2023

tustvold commented Mar 17, 2023

View reviewed changes

alamb approved these changes Mar 20, 2023

View reviewed changes

viirya approved these changes Mar 20, 2023

View reviewed changes

viirya reviewed Mar 20, 2023

View reviewed changes

Review feedback

efc9984

tustvold merged commit 67fe807 into apache:master Mar 21, 2023

spebern pushed a commit to spebern/arrow-rs that referenced this pull request Mar 25, 2023

Flesh out NullBuffer abstraction (apache#3880) (apache#3885)

f59c889

* Flesh out NullBuffer abstraction (apache#3880) * Review feedback

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flesh out NullBuffer abstraction (#3880) #3885

Flesh out NullBuffer abstraction (#3880) #3885

tustvold commented Mar 17, 2023

tustvold Mar 17, 2023

tustvold Mar 17, 2023

tustvold Mar 17, 2023

tustvold Mar 17, 2023

alamb left a comment •

edited

Loading

alamb Mar 20, 2023

alamb Mar 20, 2023

tustvold Mar 21, 2023

alamb Mar 20, 2023

alamb Mar 20, 2023

viirya left a comment

viirya Mar 20, 2023

tustvold Mar 20, 2023

tustvold commented Mar 21, 2023

	pub fn combine_option_bitmap(
	arrays: &[&ArrayData],
	len_in_bits: usize,
	) -> Option<Buffer> {
	let (buffer, offset) = arrays
	.iter()
	.map(\|array\| match array.nulls() {
	Some(n) => (Some(n.buffer().clone()), n.offset()),
	None => (None, 0),
	})
	.reduce(\|acc, buffer_and_offset\| match (acc, buffer_and_offset) {
	((None, _), (None, _)) => (None, 0),
	((Some(buffer), offset), (None, _)) \| ((None, _), (Some(buffer), offset)) => {
	(Some(buffer), offset)
	}
	((Some(buffer_left), offset_left), (Some(buffer_right), offset_right)) => (
	Some(buffer_bin_and(
	&buffer_left,
	offset_left,
	&buffer_right,
	offset_right,
	len_in_bits,
	)),
	0,
	),
	})?;

	Some(buffer?.bit_slice(offset, len_in_bits))

Flesh out NullBuffer abstraction (#3880) #3885

Flesh out NullBuffer abstraction (#3880) #3885

Conversation

tustvold commented Mar 17, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Mar 21, 2023

alamb left a comment •

edited

Loading