Add COW conversion for Buffer and PrimitiveArray and unary_mut #3115

viirya · 2022-11-15T02:43:14Z

Which issue does this PR close?

Part of #1981.

Rationale for this change

I've played around with the code to add some initial APIs for copy-on-write support. The COW support can benefit for our use cases to reduce memory allocation cost. I'd like to add some initial support to move it forward.

What changes are included in this PR?

This change is not fully COW support but adds some initial APIs for that.

Add COW conversion from Buffer to MutableBuffer
Add COW conversion from PrimitiveArray to corresponding builder
Add unary_mut to PrimitiveArray for mutating its content

Are there any user-facing changes?

tustvold · 2022-11-15T03:26:52Z

arrow-array/src/array/mod.rs

+/// Downcasts an Arc-ed sized `Array` to Arc-ed underlying arrow type.
+#[allow(dead_code)]
+pub(crate) fn downcast_array<T: Send + Sync + 'static>(
+    array: Arc<dyn SizedArray>,


How do you go from an ArrayRef to Arc<dyn SizedArray>?

Edit: Is the intention to change ArrayRef to be Arc<dyn SizedArray>? 🤔

Cannot make AsDynAny work with Arc<dyn Array>. We have implemented Array for reference type &'a T. Any must be 'static and and it must be Sized as well.

Oh, got what you mean. Hmm, good question. I currently don't make it work with ArrayRef.

Proposal in #3117

tustvold · 2022-11-15T03:28:26Z

FYI there is some overlap with #2902

tustvold · 2022-11-15T20:33:54Z

arrow-buffer/src/buffer/immutable.rs

+        let offset = self.offset;
+        let length = self.length;
+        Arc::try_unwrap(self.data)
+            .map(|bytes| {


We need to verify that bytes has deallocation of Deallocation::Arrow, otherwise this is not well formed

tustvold · 2022-11-15T20:40:26Z

arrow-buffer/src/buffer/immutable.rs

+        let length = self.length;
+        Arc::try_unwrap(self.data)
+            .map(|bytes| {
+                // The pointer of underlying buffer should not be offset.


This is a somewhat annoying limitation, I wonder if there is some way to avoid it 🤔

Perhaps we could push the offset into Bytes 🤔

For the limitation, I assume that a non-zero offset means the Bytes is shared/sliced from others. So it is disallowed to be mutable here.

Wondering pushing the offset into Bytes can change it?

Yeah ideally we'd do something like make MutableArray hold a Bytes, and then push an offset into Bytes. We could even extend the Allocation trait to allow reallocation of custom allocated data. Not sure how much, if any, of that you would be interested in doing 😅 was more just an observation

For the limitation, I assume that a non-zero offset means the Bytes is shared/sliced from others. So it is disallowed to be mutable here.

Just question: i think Arc::try_unwrap has check the exactly one strong reference.
And all sliced use the clone(), So this is impossible panic right? 🤔

Clone on an Arc increments the strong count, it does not perform a deep copy

@tustvold Sorry another question: I think in datafusion most PrimitiveArray data have only one reference during query. So base on this, most unary change using unary_mut like +1, will avoid memcpy than before using unary ?

We would likely need to update the kernels to make use of this, but theoretically. Whether this will make a major difference in practice I'm not sure, sorts, aggregates and joins, tend to dominate queries in my experience

tustvold · 2022-11-15T20:42:18Z

arrow-array/src/array/primitive_array.rs

+    /// assert_eq!(c, Int32Array::from(vec![Some(11), Some(15), None]));
+    /// # }
+    /// ```
+    pub fn unary_mut<F>(self, op: F) -> Result<PrimitiveArray<T>, ArrowError>


I wonder if this should return Self as an error to match the other similar methods?

tustvold · 2022-11-15T20:43:24Z

arrow-array/src/builder/null_buffer_builder.rs

@@ -42,6 +42,15 @@ impl NullBufferBuilder {
        }
    }

+    pub fn new_from_buffer(buffer: Option<MutableBuffer>, capacity: usize) -> Self {


I don't think the option here is necessary, as if there is no MutableBuffer, callers can just call the regular constructor?

tustvold · 2022-11-15T20:44:33Z

arrow-buffer/src/buffer/mutable.rs

@@ -92,6 +92,15 @@ impl MutableBuffer {
        }
    }

+    /// Allocates a new [MutableBuffer] from given pointer `ptr`, `capacity`.


I think it would be "safer" for the to be from_bytes and contain the necessary logic to make that well-formed, i.e. check the deallocation is as expected, etc...

tustvold · 2022-11-15T20:46:45Z

arrow-data/src/data.rs

@@ -387,6 +387,10 @@ impl ArrayData {
        &self.buffers[..]
    }

+    pub fn get_buffers(self) -> Vec<Buffer> {


What about child_data, null_buffers, etc... ?

I wonder if a more general pattern would be to use ArrayData::buffers, clone the desired buffers and then drop the ArrayData?

tustvold · 2022-11-16T02:47:35Z

arrow-array/src/array/primitive_array.rs

+        let null_count = self.null_count();
+        let null_buffer = data.null_buffer().map(|b| b.bit_slice(data.offset(), len));
+
+        let buffer = self.data.buffers()[0].clone();


I think we need call slice_with_length to apply the offset, something like

let element_len = std::mem::size_of::<T::Native>(); let buffer = self.data.buffers()[0].slice_with_length(data.offset() * element_len, len * element_len)

tustvold

This is really exciting, left some further comments. I had envisaged that into_builder and related APIs would keep the existing values. Effectively they are a way to go from the immutable Array representation back to the mutable Builder representation?

tustvold · 2022-11-16T02:50:59Z

arrow-array/src/array/primitive_array.rs

+        let len = self.len();
+        let null_bit_buffer = self.data.null_buffer().cloned();
+
+        let buffer = self.data.buffers()[0].clone();


Same thing here, I think this needs to call slice_with_length

I changed to slice_with_length but I'm wondering what's difference than clone directly here?

arrow-array/src/array/primitive_array.rs

tustvold · 2022-11-16T02:52:32Z

arrow-buffer/src/buffer/immutable.rs

+
+    /// Returns `MutableBuffer` for mutating the buffer if this buffer is not shared.
+    /// Returns `Err` if this is shared or its allocation is from an external source.
+    pub fn into_mutable(self, len: usize) -> Result<MutableBuffer, Self> {


Why does this take a len? How does this differ from Buffer::length?

Same as #3115 (comment)

tustvold · 2022-11-16T02:53:14Z

arrow-buffer/src/buffer/mutable.rs

@@ -92,6 +93,23 @@ impl MutableBuffer {
        }
    }

+    /// Allocates a new [MutableBuffer] from given `Bytes`.
+    pub(crate) fn from_bytes(bytes: Bytes, len: usize) -> Result<Self, Bytes> {


How does the provided len different from Bytes::len?

That's due to different thoughts about the API. Previously I made the builder starting from length 0. But for unary_mut I want to make a slice with same length as the array before. So this is parameterized.

Now it is changed for your suggestion so this doesn't take a len parameter now.

tustvold · 2022-11-16T02:55:33Z

arrow-array/src/builder/buffer_builder.rs

+    pub fn new_from_buffer(buffer: MutableBuffer) -> Self {
+        Self {
+            buffer,
+            len: 0,


Suggested change

len: 0,

len: buffer.len() / std::mem::size_of::<T::Native>(),

?

tustvold · 2022-11-16T02:55:55Z

arrow-array/src/builder/boolean_buffer_builder.rs

@@ -33,6 +33,10 @@ impl BooleanBufferBuilder {
        Self { buffer, len: 0 }
    }

+    pub fn new_from_buffer(buffer: MutableBuffer) -> Self {


I think this need to be provided with the length in bits?

Yes, added.

Oh, missed it.

tustvold · 2022-11-16T02:56:27Z

arrow-array/src/array/primitive_array.rs

+        builder.append_null();
+        builder.append_value(2);
+
+        let expected: Int32Array = vec![Some(4), None, Some(2)].into_iter().collect();


I would have expected into_builder to keep the current values?

Yea, currently the returned builder sets length to 0 so it will overwrite the existing values. Per the latest suggestion, it makes more sense to have the same length as the array. And we can ask for mutable slice if we want to mutate the existing values.

viirya · 2022-11-16T05:05:45Z

This is really exciting, left some further comments. I had envisaged that into_builder and related APIs would keep the existing values. Effectively they are a way to go from the immutable Array representation back to the mutable Builder representation?

Yes, they can. unary_mut demonstrates it. You can give it a op which returns original values for all or partial indexes to keep the existing values.

For into_builder, PrimitiveBuilder currently doesn't offer the capacity to advance without setting its value buffer. For keeping the existing values, we may need to add an API for advancing without changing buffer values.

Thanks for your review!

tustvold · 2022-11-16T05:08:10Z

currently doesn't offer the capacity to advance without setting its value buffer

This is by design, as it would allow for unitialised memory. My suggestion is that into_builder would create a builder with the same length as the array from which it was created, i.e. array.into_builder().finish() would be a no-op. Does that make sense?

You would then be able to implement things like unary_mut by doing something like

let mut builder = array.into_builder()?;
builder.as_mut_slice().for_each(op);
builder.finish()

viirya · 2022-11-16T05:37:28Z

This is by design, as it would allow for unitialised memory. My suggestion is that into_builder would create a builder with the same length as the array from which it was created, i.e. array.into_builder().finish() would be a no-op. Does that make sense?

You would then be able to implement things like unary_mut by doing something like
let mut builder = array.into_builder()?;
builder.as_mut_slice().for_each(op);
builder.finish()

This can work as like currently unary_mut does that iterating existing values as slice and mutating values as like.

One minor disadvantage of making the same length for the builder is, you cannot use builder's APIs (append_value , etc.) to simply overwrite the existing values.

Calling the builder's APIs like append_value will continue from the end of the existing array instead of mutating the existing values.

The builder API cannot read existing values. So it cannot be used with an op which reads existing values and mutates that.

From the point of view, the suggestion of as_mut_slice makes sense to me.

tustvold · 2022-11-16T05:41:10Z

Aah yes, my mistake I thought I had already added that, it seems I didn't get further than adding as_slice_mut to BufferBuilder

tustvold

I think a test of the null behaviour would be good, along with potentially one of the slicing behaviour (which I think will currently fail, which is fine).

arrow-array/src/array/primitive_array.rs

tustvold · 2022-11-16T06:39:59Z

arrow-array/src/array/primitive_array.rs

+        let null_bit_buffer = self.data.null_buffer().cloned();
+
+        let buffer = self.data.buffers()[0]
+            .slice_with_length(0, len * std::mem::size_of::<T::Native>());


The reason I suggested using slice_with_length was because the null buffer was being sliced. This is no longer the case so we could not do this, ultimately until into_mutable supports slices this is all moot anyway

tustvold · 2022-11-16T06:41:42Z

arrow-array/src/array/primitive_array.rs

+            .data
+            .null_buffer()
+            .cloned()
+            .and_then(|b| b.into_mutable().ok());


I think this needs to be split into the part that clones the null buffer and into_mutable after data is dropped.

I also think this method should fail if the null buffer cannot be converted, where I think it currently just loses the null buffer

Yea, that's why the doc test is failed now. Looking into how to make it work.

tustvold · 2022-11-16T06:43:08Z

arrow-array/src/builder/boolean_buffer_builder.rs

@@ -33,6 +33,10 @@ impl BooleanBufferBuilder {
        Self { buffer, len: 0 }
    }

+    pub fn new_from_buffer(buffer: MutableBuffer) -> Self {


tustvold · 2022-11-16T06:44:54Z

arrow-array/src/builder/null_buffer_builder.rs

@@ -42,6 +42,16 @@ impl NullBufferBuilder {
        }
    }

+    /// Creates a new builder from a `MutableBuffer`.
+    pub fn new_from_buffer(buffer: MutableBuffer, len: usize, capacity: usize) -> Self {


I think this needs to verify that len is less than capacity. And I think capacity must be buffer.len() * 8

I think capacity should not be passed in. Simply take buffer.len() * 8 now.

tustvold · 2022-11-16T06:49:02Z

arrow-array/src/builder/primitive_builder.rs

+            .map(|buffer| {
+                NullBufferBuilder::new_from_buffer(buffer, values_builder.len(), capacity)
+            })
+            .unwrap_or_else(|| NullBufferBuilder::new(capacity));


I don't think this is correct, I think it needs to be created with a length of values_builder.len(), as opposed to a capacity based off values_buffer

Yea, good catch!

viirya · 2022-11-16T07:56:21Z

I think a test of the null behaviour would be good, along with potentially one of the slicing behaviour (which I think will currently fail, which is fine).

This has a null test now. Not sure what slicing behavior you thought? Slicing array and build builder from it?

tustvold

Just some minor things 👍

tustvold · 2022-11-16T21:38:23Z

arrow-array/src/array/primitive_array.rs

+    /// data buffer is not shared by others.
+    pub fn into_builder(self) -> Result<PrimitiveBuilder<T>, Self> {
+        let len = self.len();
+        let null_bit_buffer = self.data.null_buffer().cloned();


We need to bit_slice this to be consistent with the values buffer

tustvold · 2022-11-16T21:39:51Z

arrow-array/src/builder/boolean_buffer_builder.rs

@@ -33,6 +33,10 @@ impl BooleanBufferBuilder {
        Self { buffer, len: 0 }
    }

+    pub fn new_from_buffer(buffer: MutableBuffer, len: usize) -> Self {
+        Self { buffer, len }


I think we need to assert that len <= buffer.len() * 8

tustvold · 2022-11-16T21:41:38Z

arrow-buffer/src/buffer/immutable.rs

+        let length = self.length;
+        Arc::try_unwrap(self.data)
+            .map(|bytes| {
+                // The pointer of underlying buffer should not be offset.


We would likely need to update the kernels to make use of this, but theoretically. Whether this will make a major difference in practice I'm not sure, sorts, aggregates and joins, tend to dominate queries in my experience

tustvold · 2022-11-16T21:43:02Z

arrow-array/src/array/primitive_array.rs

+        };
+
+        let try_mutable_buffers =
+            if let Err(mutable_null_buffer) = try_mutable_null_buffer {


A match expression might be clearer

Okay, this was I took from the clippy error suggestion. 😄
I will update it to a match.

viirya · 2022-11-17T20:44:29Z

Thanks @tustvold

ursabot · 2022-11-17T20:53:35Z

Benchmark runs are scheduled for baseline = 2a065be and contender = 5bce104. 5bce104 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Add some APIs for copy-on-write support

498c74f

github-actions bot added the arrow Changes to the arrow crate label Nov 15, 2022

tustvold reviewed Nov 15, 2022

View reviewed changes

viirya mentioned this pull request Nov 15, 2022

Add downcast_array (#2901) #3117

Merged

viirya added 2 commits November 14, 2022 21:25

Merge remote-tracking branch 'upstream/master' into cow

a3ad2c5

Update

4a028e6

viirya force-pushed the cow branch from 7e25463 to 4a028e6 Compare November 15, 2022 05:38

Add unary_mut as an example

8311372

viirya force-pushed the cow branch from 1297dea to 8311372 Compare November 15, 2022 06:59

viirya changed the title ~~Add some initial APIs for copy-on-write support~~ Add COW conversion for Buffer and PrimitiveArray and unary_mut Nov 15, 2022

tustvold reviewed Nov 15, 2022

View reviewed changes

viirya force-pushed the cow branch from 888146e to e969e5f Compare November 15, 2022 23:19

For review

42f6a1c

viirya force-pushed the cow branch from e969e5f to 42f6a1c Compare November 15, 2022 23:20

tustvold reviewed Nov 16, 2022

View reviewed changes

viirya added 2 commits November 15, 2022 22:22

For review

7ff701c

For review

cee4d48

tustvold reviewed Nov 16, 2022

View reviewed changes

Fix test and more for review

7cc4d0d

viirya force-pushed the cow branch from e78aecd to 7cc4d0d Compare November 16, 2022 08:00

viirya added 2 commits November 16, 2022 00:12

Add test on sliced array

e4a55a1

Address an overlooking review.

f7fe8a1

tustvold approved these changes Nov 16, 2022

View reviewed changes

tustvold reviewed Nov 16, 2022

View reviewed changes

For review

6e3461a

viirya merged commit 5bce104 into apache:master Nov 17, 2022

	len: 0,
	len: buffer.len() / std::mem::size_of::<T::Native>(),

Add COW conversion for Buffer and PrimitiveArray and unary_mut #3115

Add COW conversion for Buffer and PrimitiveArray and unary_mut #3115

Conversation

viirya commented Nov 15, 2022 • edited

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold Nov 15, 2022 • edited

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

tustvold commented Nov 15, 2022 • edited

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

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

tustvold Nov 16, 2022 • edited

Choose a reason for hiding this comment

tustvold left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Nov 16, 2022

tustvold commented Nov 16, 2022 • edited

viirya commented Nov 16, 2022

tustvold commented Nov 16, 2022

tustvold left a 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

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 commented Nov 16, 2022

tustvold left a 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

Choose a reason for hiding this comment

viirya commented Nov 17, 2022

ursabot commented Nov 17, 2022

viirya commented Nov 15, 2022 •

edited

tustvold Nov 15, 2022 •

edited

tustvold commented Nov 15, 2022 •

edited

tustvold Nov 16, 2022 •

edited

tustvold commented Nov 16, 2022 •

edited