take list: do not collect array of pointers #229
Conversation
Codecov Report
@@ Coverage Diff @@
## main #229 +/- ##
==========================================
+ Coverage 76.93% 76.94% +0.01%
==========================================
Files 229 229
Lines 19529 19542 +13
==========================================
+ Hits 15025 15037 +12
- Misses 4504 4505 +1
Continue to review full report at Codecov.
|
| .collect::<Vec<_>>(); | ||
| Self::finish_new(arrays, inner, extend_null_bits, capacity, use_validity) | ||
| } | ||
|
|
||
| /// # Panics | ||
| /// This function panics if any of the `arrays` is not downcastable to `PrimitiveArray<T>`. | ||
| pub fn new(arrays: &[&'a dyn Array], mut use_validity: bool, capacity: usize) -> Self { |
There was a problem hiding this comment.
I think that we should make this expect a ListArray instead of &dyn Array. No need for two functions just because of a downcast, and this also makes it more explicit that there is an expectation.
fwiw, this signature is historical; e.g. GrowablePrimitive uses PrimitiveArray already.
|
I will go ahead and change all signatures in growable to make this consistent across and avoid the extra allocation altogether. |
|
Proposal in #238 |
|
I am closing this one since it was generalized by #238. Thanks a lot for the proposal, @ritchie46 ! |
This PR proposes a new constructor method on
GrowableList. This constructor is generic over&[AsRef[ListArray<O>>and can be constructed with an array overListArray.This removes a redundant vector allocation in the take kernel, where a
Vec<ListArray>was converted to aVec<&dyn Array>so that it could be passed to the constructor.