Mutable OffsetBufferBuilder #5440

doki23 · 2024-02-27T16:25:43Z

Which issue does this PR close?

Closes #5384.

Rationale for this change

See #5384

What changes are included in this PR?

Add a mutable OffsetsBuilder

Are there any user-facing changes?

No

kylebarron · 2024-02-27T16:47:51Z

arrow-buffer/src/builder/offset.rs

+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct OffsetsBuilder {
+    offsets: Vec<usize>,


If this builds a Vec<usize>, that means it's not O(1) to an OffsetBuffer? Should this be Vec<O> instead?

I agree with you. OffsetSizeTrait is proper but it's in crate arrow-array. If we want to bound O with OffsetSizeTrait we need move it into arrow-buffer.

I think having O: ArrowNativeType is fine, but you should store a Vec<O>, not Vec<usize>, and only use usize in the API. That also means you get overflow errors when pushing new elements, not on finish.

OffsetSizeTrait extends std::ops::AddAssign so that we can get next offset by offset + length.

You don't need AddAssign... You can copy to a new variable and use plain std::ops::Add right?

tustvold · 2024-03-04T04:06:51Z

arrow-buffer/src/builder/offset.rs

+
+    /// takes the builder itself and returns an [`OffsetBuffer`]
+    pub fn finish(self) -> OffsetBuffer<O> {
+        OffsetBuffer::new(ScalarBuffer::from(self.offsets))


As written this will validate the offsets again

tustvold · 2024-03-04T05:47:50Z

arrow-buffer/src/builder/offset.rs

+    #[inline]
+    pub fn push_length(&mut self, length: O) {
+        let last_offset = self.offsets.last().unwrap();
+        let next_offset = *last_offset + length;


This should be checked arithmetic if we plan to elide the check on finish

tustvold · 2024-03-04T05:52:29Z

So my understanding is there are two potential motivations for this functionality:

To provide a more idiomatic API for constructing an OffsetBuffer
To provide a more efficient API for constructing an OffsetBuffer

Currently this PR I believe addresses 1, but not really 2. I would perhaps suggest taking a look at the way OffsetBuffer::from_lengths is implemented. In particular, tracking the in-progress size using usize, incrementing it using checked arithmetic, and converting to O with usize_as. Then at the end, i.e. in OffsetsBuilder::finish, checking if the value of usize overflows O. I'd also be very interested in any benchmarks of this approach.

Finally I think I would reduce the number of methods we provide to just the bare minimum, it is always easy to add methods in future, but removing methods is hard, so we should only commit to methods with clear use-cases

tustvold · 2024-03-04T05:53:02Z

arrow-buffer/src/builder/offset.rs

+
+    /// push a length into the builder.
+    #[inline]
+    pub fn push_length(&mut self, length: O) {


I would recommend only providing a single push_length method that takes a usize, and panics on overflow. I can't think of many use-cases for appending a length specified in O, as offsets are typically derived from slices which have a length measured in usize

tustvold · 2024-03-04T05:53:32Z

arrow-buffer/src/builder/offset.rs

+    }
+
+    /// get an iterator of lengths from builder's underlying offsets
+    pub fn lengths(&self) -> impl IntoIterator<Item = O> {


What is the use-case for this?

tustvold · 2024-03-04T05:55:15Z

arrow-buffer/src/builder/offset.rs

+    /// create from offsets
+    /// caller guarantees that offsets are monotonically increasing values
+    #[inline]
+    pub fn new_unchecked(offsets: Vec<O>) -> Self {


Suggested change

pub fn new_unchecked(offsets: Vec<O>) -> Self {

pub unsafe fn new_unchecked(offsets: Vec<O>) -> Self {

Presuming we change to using unchecked in finish

tustvold · 2024-03-04T05:57:36Z

arrow-buffer/src/builder/offset.rs

+    }
+}
+
+impl<IntoIter: IntoIterator<Item = O>, O: ArrowNativeType + Add<Output = O> + Sub<Output = O>>


I think Extend or FromIterator would be more idiomatic, but I'm not sure of the use-case for this tbh

tustvold · 2024-03-04T05:57:57Z

arrow-buffer/src/builder/offset.rs

+use crate::{ArrowNativeType, OffsetBuffer, ScalarBuffer};
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct OffsetsBuilder<O: ArrowNativeType> {


Suggested change

pub struct OffsetsBuilder<O: ArrowNativeType> {

pub struct OffsetBufferBuilder<O: ArrowNativeType> {

For consistency with the other naming

tustvold · 2024-03-04T05:58:37Z

arrow-buffer/src/builder/offset.rs

+    offsets: Vec<O>,
+}
+
+/// builder for [`OffsetBuffer`]


Suggested change

/// builder for [`OffsetBuffer`]

/// Builder for [`OffsetBuffer`]

Can we capitalize the first word of doc comments please

tustvold

Can we remove the APIs in terms of O, they're confusing imo and given the intended use with slices which have usize length, I think they're hard to use

I also wonder if you saw this

In particular, tracking the in-progress size using usize, incrementing it using checked arithmetic, and converting to O with usize_as. Then at the end, i.e. in OffsetsBuilder::finish, checking if the value of usize overflows O. I'd also be very interested in any benchmarks of this approach.

tustvold · 2024-03-14T21:40:47Z

arrow-buffer/src/builder/offset.rs

+    }
+
+    /// Push a length of usize type into builder
+    pub fn push_length(&mut self, length: O) {


Suggested change

pub fn push_length(&mut self, length: O) {

pub fn push_length(&mut self, length: usize) {

tustvold · 2024-03-14T21:41:19Z

arrow-buffer/src/builder/offset.rs

+
+    /// Extend from another OffsetsBuilder.
+    /// It gets a lengths iterator from another builder and extend the builder with the iter.
+    pub fn extend_with_builder(&mut self, another: OffsetBufferBuilder<O>) {


I would remove this method, I'm not sure of the use-case for it

tustvold · 2024-03-14T21:41:41Z

arrow-buffer/src/builder/offset.rs

+    /// It gets a lengths iterator from another builder and extend the builder with the iter.
+    pub fn extend_with_builder(&mut self, another: OffsetBufferBuilder<O>) {
+        self.reserve(another.len() - 1);
+        let mut last_offset = another.offsets[0];


Btw windows(2) is a nicer pattern for this

doki23 · 2024-03-15T00:05:55Z

Can we remove the APIs in terms of O, they're confusing imo and given the intended use with slices which have usize length, I think they're hard to use

I also wonder if you saw this

In particular, tracking the in-progress size using usize, incrementing it using checked arithmetic, and converting to O with usize_as. Then at the end, i.e. in OffsetsBuilder::finish, checking if the value of usize overflows O. I'd also be very interested in any benchmarks of this approach.

I get offset by adding last offset with length, I have to convert length to O. A push_length(&mut self, length: O) helps reduce reduplicate code. @tustvold

arrow-buffer/src/builder/offset.rs

arrow-buffer/src/lib.rs

arrow-buffer/src/builder/offset.rs

doki23 · 2024-03-16T00:11:17Z

The extend function doesn't return a Result type, so I add a try_extend method. PTAL @tustvold

tustvold · 2024-03-16T01:08:00Z

I think we're getting our wires crossed slightly, I'll get a PR up showing what I'm looking for in the next couple of days

Add OffsetsBuilder

c670836

github-actions bot added the arrow Changes to the arrow crate label Feb 27, 2024

clippy & fmt

d365746

kylebarron reviewed Feb 27, 2024

View reviewed changes

doki23 added 3 commits March 3, 2024 10:56

change underlying data type from usize to O: ArrowNativeType

5f4c246

add some mem management apis

b3a4353

clippy

33c84af

doki23 marked this pull request as ready for review March 4, 2024 04:03

tustvold reviewed Mar 4, 2024

View reviewed changes

doki23 added 2 commits March 14, 2024 11:36

optimize finish and remove lengths method

9a8c3cb

impl FromIterator & Extend

3b61d88

tustvold reviewed Mar 14, 2024

View reviewed changes

update

2bf238b

tustvold reviewed Mar 15, 2024

View reviewed changes

doki23 added 3 commits March 15, 2024 12:32

update

1117419

add try_extend

e28edd6

update unit tests

8984e91

update

72c25bd

doki23 changed the title ~~Mutable OffsetsBuilder~~ Mutable OffsetBufferBuilder Mar 16, 2024

update comments of capacity and len

d4ce561

doki23 closed this Mar 19, 2024

tustvold mentioned this pull request Mar 19, 2024

Add OffsetBufferBuilder #5532

Merged

tustvold mentioned this pull request Apr 17, 2024

Add a mutable builder for Offsets #5384

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mutable OffsetBufferBuilder #5440

Mutable OffsetBufferBuilder #5440

doki23 commented Feb 27, 2024

kylebarron Feb 27, 2024 •

edited

Loading

doki23 Feb 28, 2024

kylebarron Feb 28, 2024

doki23 Mar 1, 2024

kylebarron Mar 1, 2024 •

edited

Loading

tustvold Mar 4, 2024

tustvold Mar 4, 2024

tustvold commented Mar 4, 2024 •

edited

Loading

tustvold Mar 4, 2024 •

edited

Loading

tustvold Mar 4, 2024

tustvold Mar 4, 2024

tustvold Mar 4, 2024

tustvold Mar 4, 2024 •

edited

Loading

tustvold Mar 4, 2024

tustvold left a comment •

edited

Loading

tustvold Mar 14, 2024

tustvold Mar 14, 2024

tustvold Mar 14, 2024

doki23 commented Mar 15, 2024 •

edited

Loading

doki23 commented Mar 16, 2024

tustvold commented Mar 16, 2024

	pub fn new_unchecked(offsets: Vec<O>) -> Self {
	pub unsafe fn new_unchecked(offsets: Vec<O>) -> Self {

	pub struct OffsetsBuilder<O: ArrowNativeType> {
	pub struct OffsetBufferBuilder<O: ArrowNativeType> {

	/// builder for [`OffsetBuffer`]
	/// Builder for [`OffsetBuffer`]

	pub fn push_length(&mut self, length: O) {
	pub fn push_length(&mut self, length: usize) {

Mutable OffsetBufferBuilder #5440

Mutable OffsetBufferBuilder #5440

Conversation

doki23 commented Feb 27, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

kylebarron Feb 27, 2024 • 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

kylebarron Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Mar 4, 2024 • edited Loading

tustvold Mar 4, 2024 • 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

tustvold Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold 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

doki23 commented Mar 15, 2024 • edited Loading

doki23 commented Mar 16, 2024

tustvold commented Mar 16, 2024

kylebarron Feb 27, 2024 •

edited

Loading

kylebarron Mar 1, 2024 •

edited

Loading

tustvold commented Mar 4, 2024 •

edited

Loading

tustvold Mar 4, 2024 •

edited

Loading

tustvold Mar 4, 2024 •

edited

Loading

tustvold left a comment •

edited

Loading

doki23 commented Mar 15, 2024 •

edited

Loading