Implement Array for ArrayRef, Improve as_* kernels to take &dyn Array
#1129
Conversation
alamb
changed the title
&dyn Array
| @@ -21,7 +21,7 @@ use crate::array::*; | |||
| use crate::datatypes::*; | |||
|
|
|||
| /// Force downcast ArrayRef to PrimitiveArray<T> | |||
| pub fn as_primitive_array<T>(arr: &ArrayRef) -> &PrimitiveArray<T> | |||
| pub fn as_primitive_array<T>(arr: &dyn Array) -> &PrimitiveArray<T> | |||
There was a problem hiding this comment.
As ArrayRef now implements Array all previous calls to this function will still compile and we can also pass &dyn Array as well
There was a problem hiding this comment.
FWIW this change means that calls that pass in a &ArrayRef will generate an extra dispatch as pointed out by @tustvold on #1128 (comment) -- this can be avoided by the user calling as_primitive_array(arr.as_ref()) or as_primitive_array(&*arr) if it is important to avoid the double dispatch
| @@ -148,16 +148,6 @@ impl PyArrowConvert for ArrayData { | |||
| } | |||
| } | |||
|
|
|||
| impl PyArrowConvert for ArrayRef { | |||
There was a problem hiding this comment.
this is now covered by the impl of Array for ArrayRef
Codecov Report
@@ Coverage Diff @@
## master #1129 +/- ##
==========================================
+ Coverage 82.36% 82.39% +0.02%
==========================================
Files 169 169
Lines 49962 50008 +46
==========================================
+ Hits 41152 41202 +50
+ Misses 8810 8806 -4
Continue to review full report at Codecov.
|
|
For other reviewers, there are a bunch of good comments from @tustvold on #1128 (comment) related to alternate approaches |
Which issue does this PR close?
Closes #1128
Rationale for this change
See ticket -- I am trying to make writing compute kernels more ergonomic while working on #1127
What changes are included in this PR?
impl Array for ArrayRefarr: &ArrayReftoarr: &dyn Arrayin cast kernelsAre there any user-facing changes?
Better ergonomics (users can avoid
.as_ref()in some places)