Skip to content

Replace From<Vec<_>> impls with TryFroms for FixedSizeBinaryArray#10019

Open
quantumish wants to merge 1 commit into
apache:mainfrom
quantumish:fallible-fixedbinarray-conv
Open

Replace From<Vec<_>> impls with TryFroms for FixedSizeBinaryArray#10019
quantumish wants to merge 1 commit into
apache:mainfrom
quantumish:fallible-fixedbinarray-conv

Conversation

@quantumish
Copy link
Copy Markdown

@quantumish quantumish commented May 25, 2026

Which issue does this PR close?

Rationale for this change

There isn't a clear way to fix the From<Vec<_>> implementations for FixedSizeBinaryArray that wouldn't be confusing, so making them TryFrom is a better fit since they are in genuine use across e.g. tests within the Arrow library as well as a terser way of calling FixedSizeBinaryArray::try_from_iter or FixedSizeBinaryArray::try_from_sparse_iter.

What changes are included in this PR?

  • Converts From<Vec<&[u8]>>, From<Vec<&[u8; N]>>, and From<Vec<Option<&[u8]>>> implementations for FixedSizeBinaryArray to TryFrom implementations.
  • Adds a TryFrom<Vec<Option<&[u8; N]>>> implementation for the missing combination of types.
  • Updates various test cases within the arrow/parquet libraries to use try_from().unwrap() instead of from().

Are these changes tested?

This is sort of a transparent change in that only the API for expressing failure cases has changed rather than the actual failure cases. All existing tests surrounding conversion failures have been updated to check whether a conversion has correctly failed.

Are there any user-facing changes?

Yes, this is a breaking API change since user-facing trait implementations have been replaced with different trait implementations.

@github-actions github-actions Bot added parquet Changes to the parquet crate arrow Changes to the arrow crate labels May 25, 2026
@alamb alamb added the api-change Changes to the arrow API label May 28, 2026
Copy link
Copy Markdown
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 @quantumish -- this makes sense to me and I think is reasonable to avoid panic's

If other maintainers feel otherwise, we could potentially leave the old from impls

@alamb alamb changed the title Replace From<Vec<_>> impls with TryFroms for FixedSizeBinaryArray Replace From<Vec<_>> impls with TryFroms for FixedSizeBinaryArray May 28, 2026
Copy link
Copy Markdown
Contributor

@etseidl etseidl left a comment

Choose a reason for hiding this comment

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

This does seem more correct, but elsewhere we've kept both fallible and infallible APIs, but there we could explain the difference in the docs. Here, as @quantumish notes, From mandates a perfect infallible conversion, which the current code breaks.

TLDR I don't think we need to keep the old impls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-change Changes to the arrow API arrow Changes to the arrow crate parquet Changes to the parquet crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FixedSizeBinaryArray implements From<Vec<&[u8]>> etc despite conversion being fallible

3 participants