Add append_many to dictionary arrays to allow adding repeated values #6534
Conversation
There was a problem hiding this comment.
I wonder if you've given thought to instead providing a way to manually append keys and manually add/lookup values in the dictionary? I think this would be more flexible, and would avoid needing to add nullable variants as an added bonus. The append_key method would then be an obvious candidate to be made inline
| pub fn append(&mut self, value: impl AsRef<T::Native>) -> Result<K::Native, ArrowError> { | ||
| self.append_many(value, 1) |
There was a problem hiding this comment.
Perhaps we could instead extract the entry logic into a private member function, I'd be less concerned about potential performance regressions that way, LLVM does funny things with integer loops
| @@ -208,7 +208,21 @@ where | |||
| /// value is appended to the values array. | |||
| /// | |||
| /// Returns an error if the new index would overflow the key type. | |||
| #[inline] | |||
There was a problem hiding this comment.
What is the motivation for this, is there some benchmark you've used? Given how problematic dictionary codegen already is, because everything is generated for each key-value combination, I'm rather wary of this
There was a problem hiding this comment.
I'm happy to remove it
| pub fn append_option_many(&mut self, value: Option<impl AsRef<T::Native>>, count: usize) { | ||
| match value { | ||
| None => { | ||
| for _ in 0..count { |
There was a problem hiding this comment.
https://docs.rs/arrow-array/latest/arrow_array/builder/struct.PrimitiveBuilder.html#method.append_nulls will be significantly faster
|
@tustvold inspired the the existence of https://docs.rs/arrow-array/latest/arrow_array/builder/struct.PrimitiveBuilder.html#method.append_nulls I added methods such that we can now push the repetition all the way down into the buffers, which I think is as efficient as it's going to get. |
arrow-buffer/src/builder/null.rs
Outdated
| /// Appends `n` `true`s into the builder | ||
| /// to indicate that these `n` items are not null. | ||
| #[inline] | ||
| pub fn append_n_non_null(&mut self, n: usize) { |
There was a problem hiding this comment.
I believe this method already exists a few lines up
adriangb
force-pushed
the
append_multiple
branch
from
82c74d0
to
877e44a
Compare
|
Merge conflicts fixed and API removed! |
| @@ -202,6 +202,13 @@ impl<T: ArrowPrimitiveType> PrimitiveBuilder<T> { | |||
| self.values_builder.append(v); | |||
| } | |||
|
|
|||
| /// Appends a value of type `T` into the builder `n` times | |||
| #[inline] | |||
| pub fn append_value_repeated(&mut self, v: T::Native, n: usize) { | |||
There was a problem hiding this comment.
It is kind of sad that the naming of this can't be consistent, but I don't really have any better suggestions. Perhaps append_value_n??
There was a problem hiding this comment.
Yeah I noticed the names are getting a bit messy and mixed up. I'm happy to go with that suggestion.
|
I never addressed this:
|
Closes #6529