[GitHub] [arrow] alamb commented on a change in pull request #9017: ARROW-11038: [Rust] Removed unused trait and Result.

[GitBox <gi...@apache.org>]

alamb commented on a change in pull request #9017:
URL: https://github.com/apache/arrow/pull/9017#discussion_r549113707



##########
File path: rust/arrow/src/array/builder.rs
##########
@@ -86,160 +80,200 @@ pub(crate) fn builder_to_mutable_buffer<T: ArrowPrimitiveType>(
 /// # }
 /// ```
 #[derive(Debug)]
-pub struct BufferBuilder<T: ArrowPrimitiveType> {
+pub struct BufferBuilder<T: ArrowNativeType> {
     buffer: MutableBuffer,
     len: usize,
     _marker: PhantomData<T>,
 }
 
-/// Trait for simplifying the construction of [`Buffers`](crate::buffer::Buffer).
-///
-/// This trait is used mainly to offer separate implementations for
-/// numeric types and boolean types, while still be able to call methods on buffer builder
-/// with generic primitive type.
-/// Separate implementations of this trait allow to add implementation-details,
-/// e.g. the implementation for boolean types uses bit-packing.
-pub trait BufferBuilderTrait<T: ArrowPrimitiveType> {
+impl<T: ArrowNativeType> BufferBuilder<T> {
     /// Creates a new builder with initial capacity for _at least_ `capacity`
     /// elements of type `T`.
     ///
     /// The capacity can later be manually adjusted with the
-    /// [`reserve()`](BufferBuilderTrait::reserve) method.
+    /// [`reserve()`](BufferBuilder::reserve) method.
     /// Also the
-    /// [`append()`](BufferBuilderTrait::append),
-    /// [`append_slice()`](BufferBuilderTrait::append_slice) and
-    /// [`advance()`](BufferBuilderTrait::advance)
+    /// [`append()`](BufferBuilder::append),
+    /// [`append_slice()`](BufferBuilder::append_slice) and
+    /// [`advance()`](BufferBuilder::advance)
     /// methods automatically increase the capacity if needed.
     ///
     /// # Example:
     ///
     /// ```
-    /// use arrow::array::{UInt8BufferBuilder, BufferBuilderTrait};
+    /// use arrow::array::UInt8BufferBuilder;
     ///
     /// let mut builder = UInt8BufferBuilder::new(10);
     ///
     /// assert!(builder.capacity() >= 10);
     /// ```
-    fn new(capacity: usize) -> Self;
+    #[inline]
+    pub fn new(capacity: usize) -> Self {
+        let buffer = MutableBuffer::new(capacity * mem::size_of::<T>());
+
+        Self {
+            buffer,
+            len: 0,
+            _marker: PhantomData,
+        }
+    }
 
     /// Returns the current number of array elements in the internal buffer.
     ///
     /// # Example:
     ///
     /// ```
-    /// use arrow::array::{UInt8BufferBuilder, BufferBuilderTrait};
+    /// use arrow::array::UInt8BufferBuilder;
     ///
     /// let mut builder = UInt8BufferBuilder::new(10);
     /// builder.append(42);
     ///
     /// assert_eq!(builder.len(), 1);
     /// ```
-    fn len(&self) -> usize;
+    pub fn len(&self) -> usize {
+        self.len
+    }
 
     /// Returns whether the internal buffer is empty.
     ///
     /// # Example:
     ///
     /// ```
-    /// use arrow::array::{UInt8BufferBuilder, BufferBuilderTrait};
+    /// use arrow::array::UInt8BufferBuilder;
     ///
     /// let mut builder = UInt8BufferBuilder::new(10);
     /// builder.append(42);
     ///
     /// assert_eq!(builder.is_empty(), false);
     /// ```
-    fn is_empty(&self) -> bool;
+    pub fn is_empty(&self) -> bool {
+        self.len == 0
+    }
 
     /// Returns the actual capacity (number of elements) of the internal buffer.
     ///
     /// Note: the internal capacity returned by this method might be larger than
     /// what you'd expect after setting the capacity in the `new()` or `reserve()`
     /// functions.
-    fn capacity(&self) -> usize;
+    pub fn capacity(&self) -> usize {
+        let byte_capacity = self.buffer.capacity();
+        byte_capacity / std::mem::size_of::<T>()
+    }
 
     /// Increases the number of elements in the internal buffer by `n`
     /// and resizes the buffer as needed.
     ///
-    /// The values of the newly added elements are undefined.
+    /// The values of the newly added elements are 0.

Review comment:
       maybe `T::default()`

##########
File path: rust/arrow/src/array/mod.rs
##########
@@ -179,18 +179,17 @@ pub use self::array_string::StringOffsetSizeTrait;
 
 pub use self::builder::BooleanBufferBuilder;
 pub use self::builder::BufferBuilder;
-pub use self::builder::BufferBuilderTrait;
 
-pub type Int8BufferBuilder = BufferBuilder<Int8Type>;
-pub type Int16BufferBuilder = BufferBuilder<Int16Type>;
-pub type Int32BufferBuilder = BufferBuilder<Int32Type>;
-pub type Int64BufferBuilder = BufferBuilder<Int64Type>;
-pub type UInt8BufferBuilder = BufferBuilder<UInt8Type>;
-pub type UInt16BufferBuilder = BufferBuilder<UInt16Type>;
-pub type UInt32BufferBuilder = BufferBuilder<UInt32Type>;
-pub type UInt64BufferBuilder = BufferBuilder<UInt64Type>;
-pub type Float32BufferBuilder = BufferBuilder<Float32Type>;
-pub type Float64BufferBuilder = BufferBuilder<Float64Type>;
+pub type Int8BufferBuilder = BufferBuilder<i8>;

Review comment:
       👍  This is a nice simplification




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org