GH-49826: [Python] Return NotImplemented from Scalar/Array arithmetic dunders for unsupported types#49845
Conversation
… dunders for unsupported types
…dunders for unsupported types
|
|
|
|
|
|
1 similar comment
|
|
|
@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 Let me know if I should keep this PR or not. |
All good, no harm done ;)
Yeah, lots of sketchy things going on in these days :) Happy to see a normal response on this PR!
This is great, thanks! Will have a look tomorrow.
Yes, please do keep it. If there is no response on the other PR I plan to proceed with yours. |
AlenkaF
left a comment
There was a problem hiding this comment.
Thanks for the updates! I have circled back to the starting point regarding the catch issue. Let me know what you think.
| from collections.abc import Sequence, Mapping | ||
|
|
||
|
|
||
| def _is_valid_compute_operand(value): |
There was a problem hiding this comment.
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
arrow/python/pyarrow/error.pxi
Line 95 in 0600621
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?
There was a problem hiding this comment.
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 NotImplementedThis 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__:
arrow/python/pyarrow/scalar.pxi
Lines 165 to 169 in 4373fdf
I haven't tested those above but those are my assumptions, if someone working on the problem can validate :)
| return True | ||
|
|
||
|
|
||
| def _compute_binary_op(func_name, left, right): |
There was a problem hiding this comment.
Could we move this helper to a shared file (so that it is clearer it is used for both scalars and arrays)?
Rationale for this change
In pyarrow 24.0.0,
ScalarandArraygained arithmetic dunder methods (#32007) that unconditionally dispatch topyarrow.compute.call_function. When the other operand is an unrecognized type,_pack_compute_argsraisesTypeErrorinstead of returningNotImplemented. 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?
_compute_binary_ophelper inscalar.pxithat wrapscall_functionin atry/except TypeErrorand returnsNotImplementedon failure. This follows the same pattern already used byScalar.__eq__andArray.__eq__.Scalar(scalar.pxi) andArray(array.pxi) to use this helper.test_scalars.pyandtest_array.pyverifying 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 bothScalarandArray. 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.
ScalarandArrayarithmetic dunders now returnNotImplementedinstead of raisingTypeErrorwhen 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.
Closes [Python] Scalar arithmetic dunders raise TypeError instead of returning NotImplemented #49826