AlenkaF commented on code in PR #49845: URL: https://github.com/apache/arrow/pull/49845#discussion_r3186474733
########## python/pyarrow/scalar.pxi: ########## @@ -21,6 +21,25 @@ from uuid import UUID from collections.abc import Sequence, Mapping +def _is_valid_compute_operand(value): + if isinstance(value, (Scalar, Array, ChunkedArray, RecordBatch, Table, + list, tuple)): + return True + if np is not None and isinstance(value, np.ndarray): + return True + try: + scalar(value) + except Exception: + return False + return True + + +def _compute_binary_op(func_name, left, right): Review Comment: Could we move this helper to a shared file (so that it is clearer it is used for both scalars and arrays)? ########## python/pyarrow/scalar.pxi: ########## @@ -21,6 +21,25 @@ from uuid import UUID from collections.abc import Sequence, Mapping +def _is_valid_compute_operand(value): Review 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 https://github.com/apache/arrow/blob/060062178ca85fa2d7dbd4083574bca6f91cc44c/python/pyarrow/error.pxi#L95 This specific exception tests should already be covered in the `test_compute` so I think the [original](https://github.com/apache/arrow/pull/49845/changes/05f041952968fa46f55055938f57be9a221af8f3), 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? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
