Skip to content

GH-49826: [Python] Return NotImplemented from Scalar/Array arithmetic dunders for unsupported types - #49845

Open
alex-anast wants to merge 4 commits into
apache:mainfrom
alex-anast:alex-anast/gh-49826-python-scalar-dunder-return-notimplemented
Open

GH-49826: [Python] Return NotImplemented from Scalar/Array arithmetic dunders for unsupported types#49845
alex-anast wants to merge 4 commits into
apache:mainfrom
alex-anast:alex-anast/gh-49826-python-scalar-dunder-return-notimplemented

Conversation

@alex-anast

@alex-anast alex-anast commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

In pyarrow 24.0.0, Scalar and Array gained arithmetic dunder methods (#32007) that unconditionally dispatch to pyarrow.compute.call_function. When the other operand is an unrecognized type, _pack_compute_args raises TypeError instead of returning NotImplemented. This prevents Python from falling back to the other operand's reflected methods (__radd__, __rsub__, etc.), breaking downstream libraries that relied on this protocol.

What changes are included in this PR?

  • Adds a _compute_binary_op helper in scalar.pxi that wraps call_function in a try/except TypeError and returns NotImplemented on failure. This follows the same pattern already used by Scalar.__eq__ and Array.__eq__.
  • Updates all binary arithmetic and bitwise dunders on both Scalar (scalar.pxi) and Array (array.pxi) to use this helper.
  • Adds parametrized tests in test_scalars.py and test_array.py verifying that reflected operators on custom types are correctly invoked.

Are these changes tested?

Yes. New parametrized tests (test_dunders_return_notimplemented_for_unknown_types) cover all 10 binary operators for both Scalar and Array. The bug was also manually reproduced against pyarrow 24.0.0 to confirm the tests exercise the right code path.

Are there any user-facing changes?

Yes. Scalar and Array arithmetic dunders now return NotImplemented instead of raising TypeError when the other operand is not a recognized Arrow/NumPy type. This restores the pre-24.0.0 behavior where Python would fall back to the other operand's reflected method.

AI-generated code disclosure

This PR was developed with assistance from an AI coding tool (Claude, Anthropic). All changes have been reviewed, understood, and verified.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #49826 has been automatically assigned in GitHub to PR creator.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #49826 has been automatically assigned in GitHub to PR creator.

@AlenkaF

AlenkaF commented Apr 23, 2026

Copy link
Copy Markdown
Member

Thank you for opening a PR.
There is already one PR open: #49833 so we will proceed with that one first. The existing comments apply for both though. Also, please be upfront about the AI use in your PRs, see #49833.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #49826 has been automatically assigned in GitHub to PR creator.

1 similar comment
@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #49826 has been automatically assigned in GitHub to PR creator.

@alex-anast

Copy link
Copy Markdown
Contributor Author

@AlenkaF apologies, hadn't seen the other PR. I think there is something sketchy going on there, though, if you look at the user's profile, with e.g. ~280 git commits in a day, it might be that these PRs are fully auto-generated.

This one covers Array and also has tests, though, as you mentioned. In the most recent commit, I have also tried to address the narrower-catch comment, here.

Let me know if I should keep this PR or not.

@AlenkaF

AlenkaF commented Apr 23, 2026

Copy link
Copy Markdown
Member

@AlenkaF apologies, hadn't seen the other PR.

All good, no harm done ;)

I think there is something sketchy going on there, though, if you look at the user's profile, with e.g. ~280 git commits in a day, it might be that these PRs are fully auto-generated.

Yeah, lots of sketchy things going on in these days :) Happy to see a normal response on this PR!

This one covers Array and also has tests, though, as you mentioned. In the most recent commit, I have also tried to address the narrower-catch comment, here.

This is great, thanks! Will have a look tomorrow.

Let me know if I should keep this PR or not.

Yes, please do keep it. If there is no response on the other PR I plan to proceed with yours.

@AlenkaF AlenkaF left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the updates! I have circled back to the starting point regarding the catch issue. Let me know what you think.

Comment thread python/pyarrow/scalar.pxi Outdated
from collections.abc import Sequence, Mapping


def _is_valid_compute_operand(value):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for trying to address the narrower-catch issue here. The suggestion looks a bit more complicated than necessary, I think. So I tried to come up with possible examples to test the exceptions and came to the conclusion we always return an Arrow specific exception in the compute functions, see

cdef object convert_status(const CStatus& status):

This specific exception tests should already be covered in the test_compute so I think the original, simpler idea should actually do the job. Though adding a simple check that confirms the compute function related errors do not become NotImplemented would also be nice to add as part of the tests in this PR.

@raulcd what do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree it feels overly complex, we seem to be converting the value to scalar and then converting again when we are doing the compute function. This will penalize performance. The except Exception also seems to broad. I am wondering if simplifying everything to just something like isn't enough?

  def _compute_binary_op(func_name, left, right):                                                                      
      try:        
          return _pc().call_function(func_name, [left, right])
      except TypeError:                                                                                                
          return NotImplemented

This could swallow some internal TypeError but from my understanding kernel dispatch failures would raise ArrowNotImplementedError, (we should validate that calling a function with an unsupported type). So it would raise TypeError for operand conversion failures which is the one we want to return NotImplemented instead.
We already do something really similar with __eq__:

def __eq__(self, other):
try:
return self.equals(other)
except TypeError:
return NotImplemented

I haven't tested those above but those are my assumptions, if someone working on the problem can validate :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I looked into the kernel tests in C++ to check for any possible ArrowTypeError's that could get swallowed and found none for the kernels in question. I will revert the helper to the initial, simler version, and will add a note that it should not be used with compute functions that can throw ArrowTypeError (like cast for example).

Comment thread python/pyarrow/scalar.pxi Outdated
@github-actions github-actions Bot added awaiting committer review Awaiting committer review awaiting changes Awaiting changes and removed awaiting review Awaiting review awaiting committer review Awaiting committer review labels May 5, 2026
@AlenkaF

AlenkaF commented Jun 22, 2026

Copy link
Copy Markdown
Member

@alex-anast might you still be interested in this PR to look at the review comments?

@AlenkaF

AlenkaF commented Sep 11, 2026

Copy link
Copy Markdown
Member

As this PR, unfortunately, might get abandoned and the review did complicate things a bit (sorry for that) I plan to rebase and update the PR so it can be merged.

@AlenkaF
AlenkaF force-pushed the alex-anast/gh-49826-python-scalar-dunder-return-notimplemented branch from 05f0419 to ce93617 Compare September 11, 2026 09:55
Copilot AI lite review requested due to automatic review settings September 11, 2026 09:55
@github-actions github-actions Bot removed the awaiting changes Awaiting changes label Sep 11, 2026
@github-actions github-actions Bot added the awaiting change review Awaiting change review label Sep 11, 2026

Copilot AI left a comment

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.

🔵 Needs a closer look

The broad TypeError handling can hide genuine Arrow compute errors.

Pull request overview

Updates PyArrow Scalar and Array arithmetic dunders to return NotImplemented for unsupported operands, restoring reflected-operator fallback.

Changes:

  • Centralizes binary-operation handling.
  • Updates arithmetic and bitwise dunders.
  • Adds regression tests for reflected operators.
File summaries
File Summary
python/pyarrow/tests/test_scalars.py Tests scalar fallback behavior and errors.
python/pyarrow/tests/test_array.py Tests array fallback behavior and errors.
python/pyarrow/scalar.pxi Uses the helper for scalar binary operators.
python/pyarrow/array.pxi Uses the helper for array operators. Moderate issue: re-raise ArrowTypeError instead of converting all TypeError subclasses to NotImplemented.
Review details

Suppressed comments (1)

python/pyarrow/array.pxi:1136

  • ArrowTypeError subclasses TypeError, so this also swallows genuine compute errors. For example, _pack_compute_args accepts Table/RecordBatch, then CheckAllArrayOrScalar rejects them with ArrowTypeError; pa.array([1]) + pa.table(...) will now fall through as NotImplemented and lose the existing Arrow error. Re-raise ArrowTypeError and only convert the unsupported-argument TypeError to NotImplemented.
    try:
        return _pc().call_function(func_name, [left, right])
    except TypeError:
        return NotImplemented
  • Files reviewed: 4/4 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@AlenkaF

AlenkaF commented Sep 11, 2026

Copy link
Copy Markdown
Member

@raulcd please take a look when you have a chance, thanks!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Python] Scalar arithmetic dunders raise TypeError instead of returning NotImplemented

4 participants