Skip to content

fix(arrow-arith): return Err for shared empty buffers in binary_mut - #12012

Closed
abhinav-phi wants to merge 1 commit into
apache:mainfrom
abhinav-phi:fix/12002-binary-mut-shared-empty
Closed

abhinav-phi wants to merge 1 commit into
apache:mainfrom
abhinav-phi:fix/12002-binary-mut-shared-empty

Conversation

@abhinav-phi

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

binary_mut and try_binary_mut short-circuited empty input arrays and returned Ok without calling into_builder, so an empty array with a shared buffer was reused even though the documented contract and the unary variants return the array back as Err. The issue reports the same inconsistency.

What changes are included in this PR?

Remove the two undocumented is_empty() shortcuts in arrow-arith/src/arity.rs. Empty arrays now go through into_builder, so a shared buffer returns Err(self) and an unshared empty buffer still returns Ok.

An empty array with a shared buffer now returns Err(self) instead of Ok(Ok(empty)); this is the user-visible behaviour discussed in the issue and may warrant the api-change label.

Are these changes tested?

Added test_binary_mut_empty_shared_buffer covering both functions with shared and unshared empty inputs.

  • Focused: cargo test -p arrow-arith test_binary_mut_empty_shared_buffer passes.
  • Full crate: cargo test -p arrow-arith -> 258 unit tests and 16 doc tests pass.
  • cargo fmt --all -- --check clean.
  • cargo clippy --workspace --all-targets --all-features -- -D warnings clean.

AI assistance: yes. The change was reviewed and tested by the submitter.

Remove the empty-array shortcuts so binary_mut and try_binary_mut use the
same PrimitiveArray::into_builder buffer-sharing check as unary_mut and
try_unary_mut. Add regression coverage for shared and unshared empty inputs.
@github-actions github-actions Bot added the arrow-kernels Crates that transform arrow data (e.g. compute, cast) label Oct 31, 2026
@Rich-T-kid

Copy link
Copy Markdown
Contributor

@abhinav-phi can you enable the CI on your fork

@abhinav-phi

Copy link
Copy Markdown
Author

@Rich-T-kid enabled actions in the fork and started the CI run on the branch: https://github.com/abhinav-phi/arrow-rs/actions/runs/41263853321

i will keep an eye on it and fix anything that fails.

@alamb alamb added the Prolific Author author opened PRs or issues in more than 20 repositories in the last 30 days label Nov 2, 2026
@alamb

alamb commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Thank you for the contribution. The change may well be correct, but I am closing this PR under our guidance on AI-generated submissions.

Maintainer review time is the scarcest resource in this project. This account has opened more than 30 PRs across many repositories in the last month, which looks like GitHub "merge-maxxing" (collecting merged PRs) rather than work on a project the author uses. Several similar PRs have been closed elsewhere (for example apache/datafusion-sqlparser-rs#2788, boa-dev/temporal#760, OSGeo/grass#8126, python/cpython#158491 and wagtail/wagtail#14670). That suggests the changes aren't coming from someone who uses arrow-rs, and that no human will be around to discuss review feedback and maintain the code. Reviewing PRs like this costs us more time than it saves.

If you do use arrow-rs and this fix matters to you, please pick the one change you care most about. Then open a focused PR with a short description in your own words, one you have reviewed and understand, and we'd be happy to help with review.

@alamb alamb closed this Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrow-kernels Crates that transform arrow data (e.g. compute, cast) Prolific Author author opened PRs or issues in more than 20 repositories in the last 30 days

Projects

None yet

Development

Successfully merging this pull request may close these issues.

binary_mut and try_binary_mut return Ok for an empty array whose buffer is shared

3 participants