add Array::as_any_arc
#2902
add Array::as_any_arc
#2902
Conversation
Closes apache#2901.
| @@ -597,6 +602,10 @@ impl<'a, K: ArrowPrimitiveType, V: Sync> Array for TypedDictionaryArray<'a, K, V | |||
| self.dictionary | |||
| } | |||
|
|
|||
| fn as_any_arc(self: Arc<Self>) -> Option<Arc<dyn Any + Send + Sync + 'static>> { | |||
| None | |||
There was a problem hiding this comment.
Hmm... Perhaps we should remove the requirement for ArrayAccessor to impl Array 🤔
| @@ -258,6 +283,10 @@ impl Array for ArrayRef { | |||
| self.as_ref().as_any() | |||
| } | |||
|
|
|||
| fn as_any_arc(self: Arc<Self>) -> Option<Arc<dyn Any + Send + Sync + 'static>> { | |||
There was a problem hiding this comment.
I was really confused how this was working, and then realised this is actually implementing as_any_arc for Arc<Arc<dyn Array>> 😅
Perhaps this should return None to be consistent with as_any above which calls through to the inner array?
There was a problem hiding this comment.
as_any "above" (i.e. in the same impl) returns None. We only return None for two cases where we have a references that are not 'static (TBH I don't really like this solution but couldn't come up with something more elegant).
There was a problem hiding this comment.
as_any "above" (i.e. in the same impl) returns None
No it returns self.as_ref().as_any(). My point here is much like the other cases, we only have a reference to the underlying dyn Array here through an Arc. As written this method will return a Arc<dyn Any> that will downcast to an Arc<ArrayRef>, as opposed to as_any() which will downcast to PrimitiveArray, etc... I think this is a bit off perhaps? Given we have the ability to return None for cases where "this doesn't make sense" perhaps we could use it here?
Alternatively we could clone the inner ArrayRef and downcast that 🤔
TBH I don't really like this solution but couldn't come up with something more elegant
Ditto, hence my suggestion to hide it away in a private trait so we can revisit it later perhaps
| /// let int32array = Arc::downcast::<Int32Array>( | ||
| /// Arc::clone(batch.column(0)).as_any_arc().expect("Failed to create static Arc") | ||
| /// ).expect("Failed to downcast"); |
There was a problem hiding this comment.
I wonder if we should provide a downcast_array_ref method in cast that can abstract away this logic (and potentially allow hiding this method by putting it on a super trait within a private module)
There was a problem hiding this comment.
Adding a helpers sounds great.
Regarding privacy: do we consider Array to be a sealed trait (because this would be the implication)? Currently users can implement their own Array types if they want.
There was a problem hiding this comment.
Yes, the nature of the way that the downcasting works means that Array is effectively sealed as there would be no use to implementing it
| @@ -520,6 +521,10 @@ impl<T: ArrowPrimitiveType> Array for DictionaryArray<T> { | |||
| self | |||
| } | |||
|
|
|||
| fn as_any_arc(self: Arc<Self>) -> Option<Arc<dyn Any + Send + Sync + 'static>> { | |||
There was a problem hiding this comment.
I think this would still work with Option<Arc<dyn Any>>
There was a problem hiding this comment.
I don't get your question. Is that that you're wondering about Send + Sync + 'static? They are required for Arc::downcast.
There was a problem hiding this comment.
Oh the Send + Sync is required, but I think you can drop the 'static as it is implied by Any (and lifetime erasure for trait objects)
|
Marking as draft for now as I'm not sure what the consensus on the path forward here is. I think tcloning the ArrayData is a workable solution currently, and we can continue to provide statically typed versions of kernels to reduce the need for this (#2967) |
|
#2902 got closed using a different implementation. |
Which issue does this PR close?
Closes #2901.
Rationale for this change
Allow users to keep downcasted arrays arround. This is useful when you wanna return an array from a method for which you know the type and the caller wants to used the typed array (e.g. for certain kernels).
What changes are included in this PR?
New method
Array::as_any_arc.Are there any user-facing changes?
New method
Array::as_any_arc. Since this method is required by all implementations, this is a breaking change.