Skip to content

arrow-cast: Support cast to Dictionary<_, FixedSizeBinary> and add FixedSizeBinaryDictionaryBuilder #6666

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

Merged
merged 8 commits into from
Mar 7, 2025

Conversation

metalmatze
Copy link
Contributor

@metalmatze metalmatze commented Oct 31, 2024

Which issue does this PR close?

Closes #6665.

Rationale for this change

The added code seems to be where the casting and packing is handled.

What changes are included in this PR?

Are there any user-facing changes?

No, I don't think so.

@github-actions github-actions bot added the arrow Changes to the arrow crate label Oct 31, 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.

Can you add tests please

@alamb alamb marked this pull request as draft November 7, 2024 22:44
@alamb
Copy link
Contributor

alamb commented Nov 7, 2024

Marking as draft as this is not waiting on any more feedback -- please mark it as ready for review when it is time.

Extending the existing tests should be straightforward -- let us know if you need help @metalmatze

Thanks for the contribution

@metalmatze
Copy link
Contributor Author

Thanks, everyone!
Happy to add tests.
I couldn't find any initially, and now I can't seem to find the ones you're referencing. Are they in a different directory?

@alamb
Copy link
Contributor

alamb commented Nov 13, 2024

Thanks, everyone! Happy to add tests. I couldn't find any initially, and now I can't seem to find the ones you're referencing. Are they in a different directory?

Perhaps you could follow the model of

fn test_cast_utf8_dict() {
?

@metalmatze metalmatze force-pushed the arrow-cast-fixedsizebinary branch from 1bf1315 to c9aa2b6 Compare March 3, 2025 15:12
@metalmatze metalmatze marked this pull request as ready for review March 3, 2025 15:13
@metalmatze
Copy link
Contributor Author

I've added tests and it's ready for another round of reviews!

@alamb alamb changed the title arrow-cast: Support FixedSizeBinary packing arrow-cast: Support cast to Dictionary<_, FixedSizeBinary> and add FixedSizeBinaryDictionaryBuilder Mar 6, 2025
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 very much for this contribution @metalmatze

In order to unstall this PR as it has been sitting for so long, I took the liberty of pushing a few commits to this PR:

  1. Add test that cast deduplicated values
  2. A doc example
  3. Some fixes for clippy
  4. An error test for values of the wrong length

@metalmatze
Copy link
Contributor Author

Thank you for taking it across the finish line! Much appreciated! LGTM and it's great to learn from.

@alamb alamb merged commit 5b04ca7 into apache:main Mar 7, 2025
26 checks passed
@metalmatze metalmatze deleted the arrow-cast-fixedsizebinary branch March 7, 2025 12:54
PinkCrow007 pushed a commit to PinkCrow007/arrow-rs that referenced this pull request Mar 20, 2025
…`FixedSizeBinaryDictionaryBuilder` (apache#6666)

* arrow-cast: Support FixedSizeBinary packing

* Add fixed_size_binary_dictionary_builder.rs

* Improve tests / fmt

* clippy

* Add a documentation example

* Add test for error, improve message

* fix link in doc

---------

Co-authored-by: Andrew Lamb <[email protected]>
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.

[arrow-cast] Unsupported output type for dictionary packing: FixedSizeBinary(16)
3 participants