ARROW-12575: [R] Use unary negative kernel#10196
Conversation
3d861e7 to
09da203
Compare
| # Make it be 0 - arg | ||
| # TODO(ARROW-11950): do this in C++ compute | ||
| args <- list(0L, args[[1]]) | ||
| return(build_array_expression("negate", args[[1]])) |
There was a problem hiding this comment.
Question: do we want to use "negate" or "negate_checked" here? https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc#L514-L524
We have tended to use the "checked" versions of the other arithmetic functions, but we haven't thought too hard about it.
There was a problem hiding this comment.
I think the edge case which negate_checked watches for is probably not necessary here. @edponce
There was a problem hiding this comment.
Perhaps for the sake of consistency, using negate_checked makes sense, even if not strictly necessary?
There was a problem hiding this comment.
Alright, note that negate_checked does not support unsigned arguments:
call_function("negate_checked", args=list(Array$create(c(1,2), type=uint8())))
#> Error: NotImplemented: Function negate_checked has no kernel matching input types (array[uint8])There was a problem hiding this comment.
I just noticed that we should update the negate_checked_doc to state explicitly that it does not support unsigned arguments. We do show this in the function signature table https://github.com/apache/arrow/blob/master/docs/source/cpp/compute.rst#arithmetic-functions
There was a problem hiding this comment.
I would think that using the checked versions for language bindings is more desirable as you want errors to be triggered ASAP. The unchecked versions should be used knowing the wrap around behavior may occur. I think of it as unchecked = safe math and checked = true math
|
@thisisnic could you add some tests with multiple unary negative operators stacked up. Maybe test with odd & even numbers of them, and parens also, e.g. |
Wouldn't this just be testing R's parser? |
|
The multiple negations and parenthesis are resolved on R side as only the resolved value is passed to the C++ backend. If the expression was passed as a string then it would make sense to check these cases. When we did the C++ tests, we checked for nested negations and they passed (although these tests were not included in the final PR). |
In theory yes? I tend to have this philosophy re tests: https://twitter.com/gshotwell/status/1389563734679605251?s=20 |
No description provided.