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

feat: support casting FixedSizeList with new child type #5360

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

wjones127
Copy link
Member

Which issue does this PR close?

Closes #4426.

Rationale for this change

Makes it possible to cast between FixedSizeList arrays with different child types. We use this to be able to cast f32 vectors into f16 vectors to store them at a smaller size.

What changes are included in this PR?

Dispatch to child cast_with_options when appropriate to allow FSL -> FSL/List/LargeList with differing child types.

Are there any user-facing changes?

Yes, though I don't think we document all possible cast option at the moment.

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

@alamb alamb left a comment

Choose a reason for hiding this comment

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

The code and tests look good to me. Thank you @wjones127

While reviewing the code, I noticed the casts were in only in the FixedSizedList --> List/LargeList direction. Would it make sense to do the reverse too (List/LargeLIst --> FixedSizedLIst)

Specifically I was imagining preserving the arrays during round trip might be important (and easy to test): FixedSizeList --> List --> FixedSizedList

🤔

@tustvold tustvold merged commit ae85263 into apache:master Feb 6, 2024
25 checks passed
@alamb
Copy link
Contributor

alamb commented Feb 7, 2024

Thanks again @wjones127 and @tustvold

@wjones127
Copy link
Member Author

@alamb Sorry for not replying earlier. I did the casts for the other direction earlier in #5081. I didn't think to add a roundtrip test specifically, but I think the individual tests are specific enough to imply they will roundtrip.

@alamb
Copy link
Contributor

alamb commented Feb 8, 2024

@alamb Sorry for not replying earlier. I did the casts for the other direction earlier in #5081. I didn't think to add a roundtrip test specifically, but I think the individual tests are specific enough to imply they will roundtrip.

No worries -- makes sense to me.

@wjones127 wjones127 deleted the fast-fsl-subtypes branch February 8, 2024 03:56
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 casting from/to between FixedSizeList and List
3 participants