-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Implement a more generic from_nested_iter method for list arrays #9268
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.
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
base: main
Are you sure you want to change the base?
Conversation
Jefffrey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
from_iter_primitivecould potentially get deprecated.
I'm of the opinion to keep it (don't deprecate), as using this new API means another generic is required and from_iter_primitive is already a bit gnarly with needing 3 generics 🤔
arrow-array/src/array/list_array.rs
Outdated
| Self::from_nested_iter::<T::Native, PrimitiveBuilder<T>, P, I>(iter) | ||
| } | ||
|
|
||
| /// Creates a [`GenericListArray`] from an iterator of primitive values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to add some explanation on how this method differs from from_iter_primitive, when you would use this, etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a sentence about the supported types, and changed the example to use more complicated type than from_iter_primitive.
I also reordered the generic parameters, in most cases only the builder type needs to be specified, all others can be inferred. Still, its one more generic than from_iter_primitive, so I would agree to keep that other method un-deprecated.
Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
| /// Some(vec![Some("bar"), None, Some("foo")]), | ||
| /// Some(vec![]), | ||
| /// ]; | ||
| /// let list_array = ListArray::from_nested_iter::<StringDictionaryBuilder<Int32Type>, _, _, _>(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing
| /// let list_array = ListArray::from_nested_iter::<StringDictionaryBuilder<Int32Type>, _, _, _>(data); | ||
| /// println!("{:?}", list_array); | ||
| /// ``` | ||
| pub fn from_nested_iter<B, T, P, I>(iter: I) -> Self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe should we call it from_iter_primitive_nested to mirror from_iter_primitive ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can't have primitive in the name since this new API accepts strings too 🤔
Which issue does this PR close?
Rationale for this change
Implement a convenient function to create list arrays of many more types from nested iterators.
What changes are included in this PR?
Are these changes tested?
yes, new tests added and the existing
from_iter_primitivedelegates to the new method for additional converage.Are there any user-facing changes?
No breaking changes. The
from_iter_primitivecould potentially get deprecated.