ARROW-11950: [C++][Compute] Add unary negative kernel #10113
Conversation
|
Windows build is failing due to C4146 warning which is triggered when an unsigned integer is negated. This negation is done purposefully with the expected behavior of modulo arithmetic. An alternative is to negate unsigned integers using two's complement explicitly (see https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/basic_decimal.cc#L375): unsigned int x = 10;
unsigned int x_neg = ~x + 1; |
|
After changing the negation operation for unsigned integers, Windows build passes successfully (i.e., no warning). |
|
Same PR as #10016?? |
bkietz
left a comment
There was a problem hiding this comment.
This is looking good, thanks! A few comments
bkietz
left a comment
There was a problem hiding this comment.
This looks good, please add one test for NullType.
There was a problem hiding this comment.
Please add a test asserting the behavior when NullType is used. I expect this will emit a "no kernel found" error (acceptable), or leave the type unpromoted:
| CheckDispatchBest("negate", {null()}, {null()}); |
This would also be fine, but I'd like to see a trivial test case asserting we don't try to access absent buffers:
CheckScalarUnary("negate", NullArray(10), NullArray(10));There was a problem hiding this comment.
Using a NullType does trigger a NotImplemented error as expected, but I can't catch/assert the error from CheckDispatchBest() to pass the tests. Similar scenario for CheckScalarUnary. Any ideas?
There was a problem hiding this comment.
You could add a CheckDispatchFails(function_name, argument_types) assert function to go with CheckDispatchBest()
There was a problem hiding this comment.
I added CheckDispatchFails() which checks if status is an error after invoking either DispatchBest() or DispatchExact. It seems the only error produced by Dispatch*() is NotImplemented. Nevertheless, I made the function description general enough and check for not OK status instead of only NotImplemented error, https://github.com/edponce/arrow/blob/ARROW-11950-Compute-Add-unary-negative-kernel/cpp/src/arrow/compute/kernels/test_util.h#L151-L152
There was a problem hiding this comment.
I did not add CheckScalarUnary("negate", std::make_shared<NullArray>(10), std::make_shared<NullArray>(10)) test because it fails with NotImplemented kernel via a different path of functions. Specifically, CheckScalar() --> CallFunction() which is where the error is captured. I think adding a family of CheckScalar*Fails() function would be beneficial for testing but consider it outside the scope of this PR. Should I open an issue for extending these tests?
There was a problem hiding this comment.
PR #10098 changed a bit the behaviour of kernel error handling. Errors are not populated through KernelContext any more.
You can reference latest code in this source file. E.g., https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc#L91
There was a problem hiding this comment.
Thanks for this notice.
848e65d to
25fe7f9
Compare
This pull request adds the Negate arithmetic compute kernel for integral and floating-point types. The NegateChecked version is not implemented for unsigned integral types and overflow behavior is consistent with equivalent Add/Subtract operations. The Negate kernels are registered as "negate" and "negate_checked".
This PR also extends support for unary arithmetic compute kernels and tests.
@bkietz please review