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

Support casting StringArray/BinaryArray --> StringView / BinaryView #5686

Merged
merged 4 commits into from Apr 26, 2024

Conversation

RinChanNOWWW
Copy link
Contributor

@RinChanNOWWW RinChanNOWWW commented Apr 24, 2024

Which issue does this PR close?

Part of #5508.

Casting byte view array to byte array and casting between dictionary array and byte view array will be done in other PRs.

Rationale for this change

What changes are included in this PR?

Support casting from byte array (BinaryArray, StringArray) to byte view array (BinaryViewArray, StringVIewArray).

Are there any user-facing changes?

No.

@github-actions github-actions bot added the arrow Changes to the arrow crate label Apr 24, 2024
@alamb alamb changed the title Support casting from byte array to byte view array. Support casting from GenericByteArray to ByteArrayView Apr 26, 2024
@alamb alamb changed the title Support casting from GenericByteArray to ByteArrayView Support casting StringArray/BinaryArray --> StringView / BinaryView Apr 26, 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.

Thank you for this contribution @RinChanNOWWW -- other than using GenericByteViewArray::new_unchecked I think this PR looks wonderful.

I also think we could also merge the PR as is and then switch to new-unchecked in a follow on PR

arrow-cast/src/cast/mod.rs Outdated Show resolved Hide resolved
let value_buf = &str_values_buf[offset as usize..end as usize];
let length = end - offset;

if length <= 12 {
Copy link
Contributor

Choose a reason for hiding this comment

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

As an aside (not for this PR) it would be great to somehow encapsulate this logic into a struct to avoid having to copy the same pattern it so many times. I took a shot at this here #5619

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new enum View looks good to me.

@alamb
Copy link
Contributor

alamb commented Apr 26, 2024

Thanks @RinChanNOWWW -- I took the liberty of adding a comment to the code in this PR justifying why the unsafe was ok and plan to merge the PR when complete

@alamb
Copy link
Contributor

alamb commented Apr 26, 2024

🚀

@alamb alamb merged commit a61f1dc into apache:master Apr 26, 2024
25 checks passed
@RinChanNOWWW RinChanNOWWW deleted the cast-byte-to-view branch April 27, 2024 01:26
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.

None yet

2 participants