Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor: support cast values to fixedsizelist #5340

Merged
merged 6 commits into from
Feb 4, 2024

Conversation

Weijun-H
Copy link
Member

Which issue does this PR close?

Closes #5339

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Jan 29, 2024
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some nits

@@ -141,6 +141,8 @@ pub fn can_cast_types(from_type: &DataType, to_type: &DataType) -> bool {
}
(_, List(list_to)) => can_cast_types(from_type, list_to.data_type()),
(_, LargeList(list_to)) => can_cast_types(from_type, list_to.data_type()),
(_, FixedSizeList(list_to,size)) if size.eq(&1_i32) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(_, FixedSizeList(list_to,size)) if size.eq(&1_i32) => {
(_, FixedSizeList(list_to,size)) if *size == 1 => {

arrow-cast/src/cast.rs Outdated Show resolved Hide resolved
fn cast_values_to_fixed_size_list(
array: &dyn Array,
to: &FieldRef,
size: &i32,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
size: &i32,
size: i32,

@@ -802,6 +804,9 @@ pub fn cast_with_options(
}
(_, List(ref to)) => cast_values_to_list::<i32>(array, to, cast_options),
(_, LargeList(ref to)) => cast_values_to_list::<i64>(array, to, cast_options),
(_, FixedSizeList(ref to, size)) if size.eq(&1_i32) => {
cast_values_to_fixed_size_list(array, to, size, cast_options)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cast_values_to_fixed_size_list(array, to, size, cast_options)
cast_values_to_fixed_size_list(array, to, *size, cast_options)

let list_array = cast(&array, &DataType::LargeList(field.clone())).unwrap();
let actual = list_array
.as_any()
.downcast_ref::<LargeListArray>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use AsArray please

arrow-cast/src/cast.rs Show resolved Hide resolved
arrow-cast/src/cast.rs Show resolved Hide resolved
@tustvold tustvold changed the title Minor: support cast utf8 to fixedsizelist Minor: support cast values to fixedsizelist Jan 30, 2024
@alamb alamb merged commit 79721ec into apache:master Feb 4, 2024
25 checks passed
@alamb
Copy link
Contributor

alamb commented Feb 4, 2024

Thanks again @Weijun-H

@sadboy
Copy link
Contributor

sadboy commented May 13, 2024

Is there any particular reason we don't support the opposite -- i.e. "unwrap" a FixedSizeList<T>[1] to T? If not, I can make a PR that's basically a mirror of this one.

@alamb
Copy link
Contributor

alamb commented May 14, 2024

@sadboy it seems like a good idea to support the inverse (casting from FixedSizeList --> List/LargeList)

i am not quite sure what you mean by FixedSizeList<T>[1] to T -- perhaps you mean casting a fixed size list with size = 1 to the underlying native type? That also seems reasonable,

@sadboy
Copy link
Contributor

sadboy commented May 17, 2024

@alamb

perhaps you mean casting a fixed size list with size = 1 to the underlying native type? That also seems reasonable

Yes that's exactly what I meant. Here's PR: #5779

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support cast 'UTF8' to FixedSizeList
4 participants