Skip to content

Fix clippy warning in fixed_size_binary_array.rs#9712

Merged
alamb merged 1 commit intoapache:mainfrom
AdamGS:adamg/fix-fixed-sized-list-lint-warn
Apr 16, 2026
Merged

Fix clippy warning in fixed_size_binary_array.rs#9712
alamb merged 1 commit intoapache:mainfrom
AdamGS:adamg/fix-fixed-sized-list-lint-warn

Conversation

@AdamGS
Copy link
Copy Markdown
Contributor

@AdamGS AdamGS commented Apr 14, 2026

Which issue does this PR close?

  • Closes #NNN.

Rationale for this change

This is a future lint, so I took to the opportunity to make this it clear that the length validation only needs to happen in one case.

What changes are included in this PR?

Minor changes to validation in FixedSizeBinaryArray::try_new.

Are these changes tested?

Added some assertions to existing tests.

Are there any user-facing changes?

None

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

Thanks @AdamGS

return Err(ArrowError::InvalidArgumentError(
"Buffer cannot have non-zero length if the item size is zero".to_owned(),
));
let len = match values.len().checked_div(s) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what is the clippy lint that is triggered?

To be honest I found this formulation harder to read than the previous one

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

its going to be clippy::manual_checked_ops which I think will be part of 1.95, I just ran into it because I was building with nightly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

obviously this is a really nitpicky thing, glad to close this PR and just punt on it to whenever it becomes an actual issue

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.

Thanks @AdamGS -- I took another look at this one and I agree it makes sense. Thank you

@alamb alamb merged commit 182c7a9 into apache:main Apr 16, 2026
27 checks passed
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.

2 participants