You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/03/28 12:46:08 UTC
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3954: Add ListBuilder::append_value (#3949)
alamb commented on code in PR #3954:
URL: https://github.com/apache/arrow-rs/pull/3954#discussion_r1150549748
##########
arrow-array/src/builder/generic_list_builder.rs:
##########
@@ -125,11 +125,97 @@ where
/// Panics if the length of [`Self::values`] exceeds `OffsetSize::MAX`
#[inline]
pub fn append(&mut self, is_valid: bool) {
- self.offsets_builder
- .append(OffsetSize::from_usize(self.values_builder.len()).unwrap());
+ self.offsets_builder.append(self.next_offset());
self.null_buffer_builder.append(is_valid);
}
+ /// Returns the next offset
+ ///
+ /// # Panics
+ ///
+ /// Panics if the length of [`Self::values`] exceeds `OffsetSize::MAX`
+ #[inline]
+ fn next_offset(&self) -> OffsetSize {
+ OffsetSize::from_usize(self.values_builder.len()).unwrap()
+ }
+
+ /// Append a value to this [`GenericListBuilder`]
Review Comment:
This is great documentation -- thank you @tustvold
Can you update the following two places (the `typedef`) to point at this new documentation and remove the redundant examples?
https://github.com/apache/arrow-rs/blob/a667af8bfa57d367e7a1ec0a990d2d85bc8cc4d2/arrow-array/src/builder/mod.rs#L139-L165
https://github.com/apache/arrow-rs/blob/a667af8bfa57d367e7a1ec0a990d2d85bc8cc4d2/arrow-array/src/builder/mod.rs#L168-L198
##########
arrow-array/src/builder/generic_list_builder.rs:
##########
@@ -125,11 +125,97 @@ where
/// Panics if the length of [`Self::values`] exceeds `OffsetSize::MAX`
#[inline]
pub fn append(&mut self, is_valid: bool) {
- self.offsets_builder
- .append(OffsetSize::from_usize(self.values_builder.len()).unwrap());
+ self.offsets_builder.append(self.next_offset());
self.null_buffer_builder.append(is_valid);
}
+ /// Returns the next offset
+ ///
+ /// # Panics
+ ///
+ /// Panics if the length of [`Self::values`] exceeds `OffsetSize::MAX`
+ #[inline]
+ fn next_offset(&self) -> OffsetSize {
+ OffsetSize::from_usize(self.values_builder.len()).unwrap()
+ }
+
+ /// Append a value to this [`GenericListBuilder`]
+ ///
+ /// ```
+ /// # use arrow_array::builder::{Int32Builder, ListBuilder};
+ /// # use arrow_array::cast::AsArray;
+ /// # use arrow_array::{Array, Int32Array};
+ /// # use arrow_array::types::Int32Type;
+ /// let mut builder = ListBuilder::new(Int32Builder::new());
+ ///
+ /// builder.append_value([Some(1), Some(2), Some(3)]);
+ /// builder.append_value([]);
+ /// builder.append_value([None]);
+ ///
+ /// let array = builder.finish();
+ /// assert_eq!(array.len(), 3);
+ ///
+ /// assert_eq!(array.value_offsets(), &[0, 3, 3, 4]);
+ /// let values = array.values().as_primitive::<Int32Type>();
+ /// assert_eq!(values, &Int32Array::from(vec![Some(1), Some(2), Some(3), None]));
+ /// ```
+ ///
+ /// This is an alternative API to appending directly to [`Self::values`] and
+ /// delimiting the result with [`Self::append`]
+ ///
+ /// ```
+ /// # use arrow_array::builder::{Int32Builder, ListBuilder};
Review Comment:
I think this example is redundant with
https://github.com/apache/arrow-rs/blob/a667af8bfa57d367e7a1ec0a990d2d85bc8cc4d2/arrow-array/src/builder/mod.rs#L139-L165
##########
arrow-array/src/builder/generic_list_builder.rs:
##########
@@ -125,11 +125,97 @@ where
/// Panics if the length of [`Self::values`] exceeds `OffsetSize::MAX`
#[inline]
pub fn append(&mut self, is_valid: bool) {
- self.offsets_builder
- .append(OffsetSize::from_usize(self.values_builder.len()).unwrap());
+ self.offsets_builder.append(self.next_offset());
self.null_buffer_builder.append(is_valid);
}
+ /// Returns the next offset
+ ///
+ /// # Panics
+ ///
+ /// Panics if the length of [`Self::values`] exceeds `OffsetSize::MAX`
+ #[inline]
+ fn next_offset(&self) -> OffsetSize {
+ OffsetSize::from_usize(self.values_builder.len()).unwrap()
+ }
+
+ /// Append a value to this [`GenericListBuilder`]
+ ///
+ /// ```
+ /// # use arrow_array::builder::{Int32Builder, ListBuilder};
+ /// # use arrow_array::cast::AsArray;
+ /// # use arrow_array::{Array, Int32Array};
+ /// # use arrow_array::types::Int32Type;
+ /// let mut builder = ListBuilder::new(Int32Builder::new());
+ ///
+ /// builder.append_value([Some(1), Some(2), Some(3)]);
+ /// builder.append_value([]);
+ /// builder.append_value([None]);
+ ///
+ /// let array = builder.finish();
+ /// assert_eq!(array.len(), 3);
+ ///
+ /// assert_eq!(array.value_offsets(), &[0, 3, 3, 4]);
+ /// let values = array.values().as_primitive::<Int32Type>();
+ /// assert_eq!(values, &Int32Array::from(vec![Some(1), Some(2), Some(3), None]));
+ /// ```
+ ///
+ /// This is an alternative API to appending directly to [`Self::values`] and
+ /// delimiting the result with [`Self::append`]
+ ///
+ /// ```
+ /// # use arrow_array::builder::{Int32Builder, ListBuilder};
+ /// # use arrow_array::cast::AsArray;
+ /// # use arrow_array::{Array, Int32Array};
+ /// # use arrow_array::types::Int32Type;
+ /// let mut builder = ListBuilder::new(Int32Builder::new());
+ ///
+ /// builder.values().append_value(1);
+ /// builder.values().append_value(2);
+ /// builder.values().append_value(3);
+ /// builder.append(true);
+ /// builder.append(true);
+ /// builder.values().append_null();
+ /// builder.append(true);
+ ///
+ /// let array = builder.finish();
+ /// assert_eq!(array.len(), 3);
+ ///
+ /// assert_eq!(array.value_offsets(), &[0, 3, 3, 4]);
+ /// let values = array.values().as_primitive::<Int32Type>();
+ /// assert_eq!(values, &Int32Array::from(vec![Some(1), Some(2), Some(3), None]));
+ /// ```
+ #[inline]
+ pub fn append_value<I, V>(&mut self, i: I)
+ where
+ T: Extend<Option<V>>,
+ I: IntoIterator<Item = Option<V>>,
+ {
+ self.extend(std::iter::once(Some(i)))
+ }
+
+ /// Append a null to this [`GenericListBuilder`]
Review Comment:
I recommend adding a link to `append_value` for an example
```suggestion
/// Append a null to this [`GenericListBuilder`]
///
/// See [`Self::append_value`] for an example use.
```
##########
arrow-array/src/builder/generic_list_builder.rs:
##########
@@ -125,11 +125,97 @@ where
/// Panics if the length of [`Self::values`] exceeds `OffsetSize::MAX`
#[inline]
pub fn append(&mut self, is_valid: bool) {
- self.offsets_builder
- .append(OffsetSize::from_usize(self.values_builder.len()).unwrap());
+ self.offsets_builder.append(self.next_offset());
self.null_buffer_builder.append(is_valid);
}
+ /// Returns the next offset
+ ///
+ /// # Panics
+ ///
+ /// Panics if the length of [`Self::values`] exceeds `OffsetSize::MAX`
+ #[inline]
+ fn next_offset(&self) -> OffsetSize {
+ OffsetSize::from_usize(self.values_builder.len()).unwrap()
+ }
+
+ /// Append a value to this [`GenericListBuilder`]
+ ///
+ /// ```
+ /// # use arrow_array::builder::{Int32Builder, ListBuilder};
+ /// # use arrow_array::cast::AsArray;
+ /// # use arrow_array::{Array, Int32Array};
+ /// # use arrow_array::types::Int32Type;
+ /// let mut builder = ListBuilder::new(Int32Builder::new());
+ ///
+ /// builder.append_value([Some(1), Some(2), Some(3)]);
+ /// builder.append_value([]);
+ /// builder.append_value([None]);
+ ///
+ /// let array = builder.finish();
+ /// assert_eq!(array.len(), 3);
+ ///
+ /// assert_eq!(array.value_offsets(), &[0, 3, 3, 4]);
+ /// let values = array.values().as_primitive::<Int32Type>();
+ /// assert_eq!(values, &Int32Array::from(vec![Some(1), Some(2), Some(3), None]));
+ /// ```
+ ///
+ /// This is an alternative API to appending directly to [`Self::values`] and
+ /// delimiting the result with [`Self::append`]
+ ///
+ /// ```
+ /// # use arrow_array::builder::{Int32Builder, ListBuilder};
+ /// # use arrow_array::cast::AsArray;
+ /// # use arrow_array::{Array, Int32Array};
+ /// # use arrow_array::types::Int32Type;
+ /// let mut builder = ListBuilder::new(Int32Builder::new());
+ ///
+ /// builder.values().append_value(1);
+ /// builder.values().append_value(2);
+ /// builder.values().append_value(3);
+ /// builder.append(true);
+ /// builder.append(true);
+ /// builder.values().append_null();
+ /// builder.append(true);
+ ///
+ /// let array = builder.finish();
+ /// assert_eq!(array.len(), 3);
+ ///
+ /// assert_eq!(array.value_offsets(), &[0, 3, 3, 4]);
+ /// let values = array.values().as_primitive::<Int32Type>();
+ /// assert_eq!(values, &Int32Array::from(vec![Some(1), Some(2), Some(3), None]));
+ /// ```
+ #[inline]
+ pub fn append_value<I, V>(&mut self, i: I)
+ where
+ T: Extend<Option<V>>,
+ I: IntoIterator<Item = Option<V>>,
+ {
+ self.extend(std::iter::once(Some(i)))
+ }
+
+ /// Append a null to this [`GenericListBuilder`]
+ #[inline]
+ pub fn append_null(&mut self) {
+ self.offsets_builder.append(self.next_offset());
+ self.null_buffer_builder.append_null();
+ }
+
+ /// Appends an optional value into this [`GenericListBuilder`]
+ ///
+ /// If `Some` calls [`Self::append_value`] otherwise calls [`Self::append_null`]
Review Comment:
```suggestion
/// If `Some` calls [`Self::append_value`] otherwise calls [`Self::append_null`]
///
/// See [`Self::append_value`] for an example use.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org