Provide additional common builder interfaces with ValuesBuilder, a subtrait of ArrayBuilder. #6927
Conversation
github-actions
bot
added
parquet
| /// Builds the [`GenericByteViewArray`] and reset this builder | ||
| pub fn finish(&mut self) -> GenericByteViewArray<T> { |
There was a problem hiding this comment.
Github diff is weird. These few functions were not changed -- only moved as the ValuesBuilder was implemented below.
| fn process_regexp_array_match< | ||
| 'a, | ||
| T: ?Sized, | ||
| A: ArrayAccessor<Item = &'a str>, | ||
| O: OffsetSizeTrait, | ||
| B: ValuesBuilder<T, Value = str>, | ||
| >( | ||
| array: ArrayIter<A>, | ||
| regex_array: ArrayIter<A>, | ||
| flags_array: Option<ArrayIter<A>>, | ||
| list_builder: &mut GenericListBuilder<O, B>, | ||
| ) -> Result<(), ArrowError> { |
There was a problem hiding this comment.
Logic for this method (previous macro) is the same. It's only the generics which changed, such as providing generic ArrayIter is place of arrays which then were then converted into an ArrayIter with array.iter().
|
I've not had time to look in detail, but this sounds a lot like the Extend trait which we already support, and is more flexible as it allows appending iterators? Perhaps we just need to better document this? |
Ah yes, the initial use case (in the regex code) could have been refactored and then used the Extend trait for the generics. Thank you. Do you think its still worthwhile to have this trait anyways? Since the append interfaces (for list building) are publicly exposed for other users. Or should we only add more documentation referring users to the Extend interface? |
|
I think it would be good to better document the Extend trait. I think if we do proceed with this, we'd need to keep the member functions as well, so people don't have to import the trait - as it stands this PR is a breaking change. That being said, I'd personally be inclined to not add this, but it has been suggested before so people clearly have difficulty finding Extend. Perhaps documentation will fix that... |
|
Closing as unneeded. |
Which issue does this PR close?
There is no github issue. It was a gap noticed during a code review, resulting in the use of declarative macros in place of generic functions. The problem was that we lacked defined interfaces for common builder functionality.
Rationale for this change
The ArrayBuilder defines the minimum set of builder interfaces for runtime. Some common interfaces are not included, such as building lists of values or lists-of-lists. These interfaces are not included since the ArrayBuilder includes the minimum common set for generics, and handle very different bytes layouts (and runtime builder patterns) such as MapBuilder.
However, it may be worthwhile to define ArrayBuilder subtraits when its worthwhile for common runtime interactions -- such as building a list of values (the example done in this PR).
What changes are included in this PR?
trait ValuesBuilder: ArrayBuilder, and implement for only those byte array buildersprocess_regexp_matchwith generic functionprocess_regexp_array_matchwith generic functionAre there any user-facing changes?
A new subtrait is defined.