alamb commented on a change in pull request #9425:
URL: https://github.com/apache/arrow/pull/9425#discussion_r577131850
##########
File path: rust/arrow/src/array/array_list.rs
##########
@@ -139,7 +166,19 @@ impl<OffsetSize: OffsetSizeTrait> From<ArrayDataRef> for
GenericListArray<Offset
1,
"ListArray should contain a single child array (values array)"
);
- let values = make_array(data.child_data()[0].clone());
+
+ let values = data.child_data()[0].clone();
+
+ if let Some(child) = Self::get_type(data.data_type()) {
Review comment:
Here is a mid-way proposal:
https://github.com/apache/arrow/pull/9508/commits/a31a35a3614ebadc06c83c080440ed9fe1952fa3
Basically, to use normal rust handling, but the make the `Into`
implementation `expect` the result
I actually think using asserts / panics directly (as in this PR) is also
fine beacuse:
1. it is an improvement over the current behavior (crash / undefined) to
get useful error messages (even if it is in a panic :( )
2. the use of `ArrayData` in my mind is also an implementation detail of an
`Array` so most users of Arrow shouldn't be interacting with this code at all.
##########
File path: rust/arrow/src/array/array_list.rs
##########
@@ -139,7 +166,19 @@ impl<OffsetSize: OffsetSizeTrait> From<ArrayDataRef> for
GenericListArray<Offset
1,
"ListArray should contain a single child array (values array)"
);
- let values = make_array(data.child_data()[0].clone());
+
+ let values = data.child_data()[0].clone();
+
+ if let Some(child) = Self::get_type(data.data_type()) {
Review comment:
BTW I also tried using `TryFrom` directly and as @jorgecarleitao
suspected there are many kernel implementations that rely in this being
infallable.
##########
File path: rust/arrow/src/array/array_list.rs
##########
@@ -31,12 +31,19 @@ use crate::datatypes::*;
/// trait declaring an offset size, relevant for i32 vs i64 array types.
pub trait OffsetSizeTrait: ArrowNativeType + Num + Ord + std::ops::AddAssign {
+ fn is_large() -> bool;
+
fn prefix() -> &'static str;
fn to_isize(&self) -> isize;
}
impl OffsetSizeTrait for i32 {
+ #[inline]
+ fn is_large() -> bool {
+ false
+ }
+
fn prefix() -> &'static str {
Review comment:
Here is one way to remove `prefix` that does not go as far as
@jorgecarleitao suggests to collapse the traits...
https://github.com/apache/arrow/pull/9508/commits/8e68e05160a56f9feb64438e4dcd739a67a884e1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
