New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ListBuilder::append_value (#3949) #3954

Merged

tustvold merged 2 commits into apache:master from tustvold:add-append-to-list-builder

Mar 28, 2023

Contributor

tustvold commented Mar 26, 2023

Which issue does this PR close?

Part of #3949
Relates to #1841

Rationale for this change

The current ListBuilder API involves appending values to the ListBuilder::values() and then finishing the slot with ListBuilder::append. Whilst flexible, this API is rather verbose. Now that all the builders implement Extend it is possible to provide a more ergonomic API.

What changes are included in this PR?

Are there any user-facing changes?


          Add ListBuilder::append_value (apache#3949)

github-actions bot added the arrow label

tustvold commented

View reviewed changes

arrow-array/src/builder/generic_list_builder.rs

+                      T: Extend<Option<V>>,
+                      I: IntoIterator<Item = Option<V>>,
+                  {
+                      self.extend(std::iter::once(Some(i)))

Contributor Author

tustvold Mar 26, 2023

At least in some basic experiments LLVM seems to be able to optimise this as one would hope

https://rust.godbolt.org/z/zcxzqzf59

tustvold commented

View reviewed changes

arrow-array/src/builder/generic_list_builder.rs

+                  /// # use arrow_array::types::Int32Type;
+                  /// let mut builder = ListBuilder::new(Int32Builder::new());
+                  ///
+                  /// builder.append_value([Some(1), Some(2), Some(3)]);

Contributor Author

tustvold Mar 26, 2023

I think this is much easier to read than the current API

tustvold mentioned this pull request

Improve array builder documentation (#3949) #3951

Merged

alamb approved these changes

View reviewed changes

Contributor

alamb left a comment

I agree -- this is much nicer @tustvold -- thank you

arrow-array/src/builder/generic_list_builder.rs

+                  /// delimiting the result with [`Self::append`]
+                  ///
+                  /// ```
+                  /// # use arrow_array::builder::{Int32Builder, ListBuilder};

Contributor

alamb Mar 28, 2023

I think this example is redundant with

arrow-rs/arrow-array/src/builder/mod.rs

Lines 139 to 165 in a667af8

    
           /// # Example 
        
           /// 
        
           /// ``` 
        
           /// # use arrow_array::builder::{StringBuilder, ListBuilder}; 
        
           /// # use arrow_array::ListArray; 
        
           /// // Build a 3 element array of lists: 
        
           /// // 
        
           /// // column 
        
           /// // --------- 
        
           /// // [one] 
        
           /// // [] 
        
           /// // [two, three] 
        
           /// 
        
           /// let mut builder = ListBuilder::new(StringBuilder::new()); 
        
           /// // [one] 
        
           /// builder.values().append_value("one"); 
        
           /// builder.append(true); 
        
           /// // [] 
        
           /// builder.append(true); 
        
           /// // [two, three] 
        
           /// builder.values().append_value("two"); 
        
           /// builder.values().append_value("three"); 
        
           /// builder.append(true); 
        
           /// 
        
           /// // Create an array 
        
           /// let list_array: ListArray = builder.finish(); 
        
           /// ```

arrow-array/src/builder/generic_list_builder.rs Show resolved Hide resolved

arrow-array/src/builder/generic_list_builder.rs

+                      self.extend(std::iter::once(Some(i)))
+                  }
+                  /// Append a null to this [`GenericListBuilder`]

Contributor

alamb Mar 28, 2023

I recommend adding a link to append_value for an example

Suggested change

      
                /// Append a null to this [`GenericListBuilder`]
          
                /// Append a null to this [`GenericListBuilder`]
          
                ///
          
                /// See [`Self::append_value`] for an example use.

arrow-array/src/builder/generic_list_builder.rs

+                  /// Appends an optional value into this [`GenericListBuilder`]
+                  ///
+                  /// If `Some` calls [`Self::append_value`] otherwise calls [`Self::append_null`]

Contributor

alamb Mar 28, 2023

Suggested change

      
                /// If `Some` calls [`Self::append_value`] otherwise calls [`Self::append_null`]
          
                /// If `Some` calls [`Self::append_value`] otherwise calls [`Self::append_null`]
          
                ///
          
                /// See [`Self::append_value`] for an example use.


          Review feedback

1f66467

tustvold merged commit eb36d37 into apache:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrow