ARROW-11932: [C++] Implement ArrayBuilder::AppendScalar #10579
Conversation
cpp/src/arrow/array/builder_base.h
Outdated
There was a problem hiding this comment.
I would also suggest the overload AppendScalar(const Scalar& scalat, int64_t nrepeats) to append the same scalar a number of times.
There was a problem hiding this comment.
Good point, I've added the overload.
|
Hmm, it looks like the implementation could be more optimized. When appending multiple scalars (or multiple times the same scalars), the required space could be reserved. Also, at least for primitive types, I would expect appending multiple scalars to be much more streamlined than this. (it looks like such optimizations could be crafted more easily if Perhaps create a TODO JIRA for those optimizations, unless you would like to work on them on this PR. |
|
I changed it to at least call Reserve() since that's simple. I filed ARROW-13197 and also noted the dictionary scalar support there. |
|
(Note: Ben left feedback on this in #10511 so I'll address that here.) |
|
Now fixed to not require allocations/not use a vector as Ben suggested. |
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
cpp/src/arrow/array/builder_base.cc
Outdated
| namespace { | ||
| struct AppendScalarImpl { | ||
| template <typename T> | ||
| enable_if_t<has_c_type<T>::value || is_decimal_type<T>::value, Status> Visit(const T&) { |
There was a problem hiding this comment.
This doesn't include FixedSizeBinaryType, which doesn't require a ReserveData call
There was a problem hiding this comment.
FixedSizeBinaryType didn't work with this specialization anyways, I'll go add the required Builder::Append(Buffer) call that's needed.
|
This got included in #10511. |
3 participants
This repurposes the code from ScalarVectorToArray. This doesn't support dictionary scalars.