asubiotto commented on code in PR #8735:
URL: https://github.com/apache/arrow-rs/pull/8735#discussion_r2477534860
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -864,9 +877,41 @@ pub fn cast_with_options(
let array = array.as_list::<i64>();
cast_list_to_fixed_size_list::<i64>(array, field, *size,
cast_options)
}
- (List(_) | LargeList(_), _) => match to_type {
+ (ListView(_), List(_)) => match to_type {
+ List(list_to) => cast_list_view_values::<i32>(array, list_to,
cast_options),
+ _ => Err(ArrowError::CastError(
+ "Cannot cast list view to non-list data types".to_string(),
+ )),
+ },
+ (LargeListView(_), LargeList(_)) => match to_type {
Review Comment:
`match to_type` here is redundant; the second value of the tuple you're
matching is already `LargeList` here and elsewhere where the second tuple
matcher is not `_`
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -153,7 +153,20 @@ pub fn can_cast_types(from_type: &DataType, to_type:
&DataType) -> bool {
(List(list_from) | LargeList(list_from), FixedSizeList(list_to, _)) =>
{
can_cast_types(list_from.data_type(), list_to.data_type())
}
+ (List(list_from), ListView(list_to)) => {
Review Comment:
I think (based on earlier match cases), you can just write two matches:
```
(List(list_from) | LargeList(list_from), ListView(list_to) |
LargeListView(list_to))
```
and vice-versa
##########
arrow-cast/src/cast/list.rs:
##########
@@ -160,6 +169,48 @@ pub(crate) fn cast_list_values<O: OffsetSizeTrait>(
)?))
}
+/// Helper function to cast the values in a list view to a list
+pub(crate) fn cast_list_view_values<O: OffsetSizeTrait>(
Review Comment:
I think the name of this function is a misnomer because we're actually
casting a list view to a list, not just the values. I think it should be
renamed to `cast_list_view_to_list`. `cast_list_view_values` should be a
different function that casts the inner element types while leaving list_view
alone. I believe these cases are missing from your match arms (i.e.
`ListView(list_from), ListView(list_to)`). While we're at it, we should
probably also support `ListView(list_from), LargeListView(list_to)`, basically
try to mirror all the list functionality as much as possible.
##########
arrow-cast/src/cast/list.rs:
##########
@@ -160,6 +169,48 @@ pub(crate) fn cast_list_values<O: OffsetSizeTrait>(
)?))
}
+/// Helper function to cast the values in a list view to a list
+pub(crate) fn cast_list_view_values<O: OffsetSizeTrait>(
Review Comment:
This seems reasonable to me
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -864,9 +877,41 @@ pub fn cast_with_options(
let array = array.as_list::<i64>();
cast_list_to_fixed_size_list::<i64>(array, field, *size,
cast_options)
}
- (List(_) | LargeList(_), _) => match to_type {
+ (ListView(_), List(_)) => match to_type {
+ List(list_to) => cast_list_view_values::<i32>(array, list_to,
cast_options),
+ _ => Err(ArrowError::CastError(
+ "Cannot cast list view to non-list data types".to_string(),
+ )),
+ },
+ (LargeListView(_), LargeList(_)) => match to_type {
+ LargeList(list_to) => cast_list_view_values::<i64>(array, list_to,
cast_options),
+ _ => Err(ArrowError::CastError(
+ "Cannot cast list view to non-list data types".to_string(),
+ )),
+ },
+ (List(list_from) | LargeList(list_from), _) => match to_type {
Utf8 => value_to_string::<i32>(array, cast_options),
LargeUtf8 => value_to_string::<i64>(array, cast_options),
+ ListView(list_to) => {
Review Comment:
I don't think these match arms belong here, they should be top-level arms
`List -> ListView` and `LargeList -> LargeListView`
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -864,9 +877,41 @@ pub fn cast_with_options(
let array = array.as_list::<i64>();
cast_list_to_fixed_size_list::<i64>(array, field, *size,
cast_options)
}
- (List(_) | LargeList(_), _) => match to_type {
+ (ListView(_), List(_)) => match to_type {
+ List(list_to) => cast_list_view_values::<i32>(array, list_to,
cast_options),
+ _ => Err(ArrowError::CastError(
+ "Cannot cast list view to non-list data types".to_string(),
+ )),
+ },
+ (LargeListView(_), LargeList(_)) => match to_type {
+ LargeList(list_to) => cast_list_view_values::<i64>(array, list_to,
cast_options),
+ _ => Err(ArrowError::CastError(
+ "Cannot cast list view to non-list data types".to_string(),
+ )),
+ },
+ (List(list_from) | LargeList(list_from), _) => match to_type {
Utf8 => value_to_string::<i32>(array, cast_options),
LargeUtf8 => value_to_string::<i64>(array, cast_options),
+ ListView(list_to) => {
+ if list_to.data_type() != list_from.data_type() {
+ // To transform inner type, can first cast to ListView
with new inner type.
Review Comment:
I don't fully understand why we're doing this check. Isn't "castability"
checked before this?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
