Add AsArray trait for more ergonomic downcasting #3912
Conversation
github-actions
bot
added
arrow
| } | ||
|
|
||
| /// Downcast this to a [`GenericByteArray`] returning `None` if not possible | ||
| fn as_bytes_opt<T: ByteArrayType>(&self) -> Option<&GenericByteArray<T>>; |
There was a problem hiding this comment.
In hindsight I probably should have called this GenericBytesArray but hey ho
There was a problem hiding this comment.
LGTM
Some other suggestions:
- Mark
as_XXX_arrayfunctions deprecated (or maybe make a note to do so in a later release) - Consider updating the example in the main README: https://docs.rs/arrow/35.0.0/arrow/#type-erasure--trait-objects
| @@ -368,7 +368,7 @@ pub type LargeStringRunBuilder<K> = GenericByteRunBuilder<K, LargeUtf8Type>; | |||
| /// | |||
| /// // Values are polymorphic and so require a downcast. | |||
| /// let av = array.values(); | |||
| /// let ava: &BinaryArray = as_generic_binary_array::<i32>(av.as_ref()); | |||
| /// let ava: &BinaryArray = av.as_binary(); | |||
There was a problem hiding this comment.
this is much nicer -- I hit this with @appletreeisyellow when trying to downcast to a BinaryArray (having to know to put i32 was non obvious)
There was a problem hiding this comment.
Err... The i32 is just being inferred because of the : &BinaryArray type constraint...
There was a problem hiding this comment.
I am thinking as a user here 🤔
There was a problem hiding this comment.
There is also as_bytes::<Utf8Type>() / as_bytes::<BinaryType>, etc... if that is more obvious for users? The as_binary and as_string are just short-cuts for the common case of only caring about one
Edit: Ultimately I'm not sure there is a way to abstract away caring about the offsets, if you're dealing with variable length types, be they strings, binary or lists, you need to type the offset...
There was a problem hiding this comment.
I don't necessarily think the code needs to change. I think what helps the most is having some examples around that you can start from. So as always more docs / examples I think the better
| let left = as_generic_binary_array::<i64>(left); | ||
| lt_eq_binary_scalar(left, right) | ||
| } | ||
| DataType::Binary => lt_eq_binary_scalar(left.as_binary::<i32>(), right), |
| /// ``` | ||
| pub trait AsArray: private::Sealed { | ||
| /// Downcast this to a [`BooleanArray`] returning `None` if not possible | ||
| fn as_boolean_opt(&self) -> Option<&BooleanArray>; |
There was a problem hiding this comment.
I like how you have provided both _opt and non _opt variants so users can choose
| impl private::Sealed for dyn Array + '_ {} | ||
| impl AsArray for dyn Array + '_ { | ||
| fn as_boolean_opt(&self) -> Option<&BooleanArray> { | ||
| self.as_any().downcast_ref() |
I will make a note to do so in a future PR |
Which issue does this PR close?
Closes #.
Rationale for this change
We currently provide downcasting utility functions, however, they have a couple of issues:
as_largestring_arraybutas_large_list_arrayas_byte_array&ArrayRefresult in two dynamic dispatches, asArrayRefis itself coerced todyn Array&as necessaryWhat changes are included in this PR?
Are there any user-facing changes?