You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/29 10:48:27 UTC
[GitHub] [arrow] thisisnic opened a new pull request #10196: ARROW-12575: [R] Use unary negative kernel
thisisnic opened a new pull request #10196:
URL: https://github.com/apache/arrow/pull/10196
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10196: ARROW-12575: [R] Use unary negative kernel
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10196:
URL: https://github.com/apache/arrow/pull/10196#discussion_r625890460
##########
File path: r/R/expression.R
##########
@@ -56,13 +56,9 @@ build_array_expression <- function(FUN,
args = list(...),
options = empty_named_list()) {
if (FUN == "-" && length(args) == 1L) {
- # Unary -, i.e. just make it negative, and somehow this works
if (inherits(args[[1]], c("ArrowObject", "array_expression"))) {
- # 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]]))
Review comment:
I think the edge case which `negate_checked` watches for is probably not necessary here. @edponce
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] edponce commented on pull request #10196: ARROW-12575: [R] Use unary negative kernel
Posted by GitBox <gi...@apache.org>.
edponce commented on pull request #10196:
URL: https://github.com/apache/arrow/pull/10196#issuecomment-832237133
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).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #10196: ARROW-12575: [R] Use unary negative kernel
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #10196:
URL: https://github.com/apache/arrow/pull/10196#discussion_r625885713
##########
File path: r/R/expression.R
##########
@@ -56,13 +56,9 @@ build_array_expression <- function(FUN,
args = list(...),
options = empty_named_list()) {
if (FUN == "-" && length(args) == 1L) {
- # Unary -, i.e. just make it negative, and somehow this works
if (inherits(args[[1]], c("ArrowObject", "array_expression"))) {
- # 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]]))
Review 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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #10196: ARROW-12575: [R] Use unary negative kernel
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10196:
URL: https://github.com/apache/arrow/pull/10196#discussion_r626093199
##########
File path: r/R/expression.R
##########
@@ -56,13 +56,9 @@ build_array_expression <- function(FUN,
args = list(...),
options = empty_named_list()) {
if (FUN == "-" && length(args) == 1L) {
- # Unary -, i.e. just make it negative, and somehow this works
if (inherits(args[[1]], c("ArrowObject", "array_expression"))) {
- # 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]]))
Review 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
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #10196: ARROW-12575: [R] Use unary negative kernel
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10196:
URL: https://github.com/apache/arrow/pull/10196#discussion_r626093317
##########
File path: r/R/expression.R
##########
@@ -56,13 +56,9 @@ build_array_expression <- function(FUN,
args = list(...),
options = empty_named_list()) {
if (FUN == "-" && length(args) == 1L) {
- # Unary -, i.e. just make it negative, and somehow this works
if (inherits(args[[1]], c("ArrowObject", "array_expression"))) {
- # 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]]))
Review 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
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #10196: ARROW-12575: [R] Use unary negative kernel
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #10196:
URL: https://github.com/apache/arrow/pull/10196#issuecomment-832175600
@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.
```
---2
-(-(-2))
--2
-(-(2))
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #10196: ARROW-12575: [R] Use unary negative kernel
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10196:
URL: https://github.com/apache/arrow/pull/10196#discussion_r626092388
##########
File path: r/R/expression.R
##########
@@ -56,13 +56,9 @@ build_array_expression <- function(FUN,
args = list(...),
options = empty_named_list()) {
if (FUN == "-" && length(args) == 1L) {
- # Unary -, i.e. just make it negative, and somehow this works
if (inherits(args[[1]], c("ArrowObject", "array_expression"))) {
- # 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]]))
Review 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*.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10196: ARROW-12575: [R] Use unary negative kernel
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10196:
URL: https://github.com/apache/arrow/pull/10196#discussion_r625957033
##########
File path: r/R/expression.R
##########
@@ -56,13 +56,9 @@ build_array_expression <- function(FUN,
args = list(...),
options = empty_named_list()) {
if (FUN == "-" && length(args) == 1L) {
- # Unary -, i.e. just make it negative, and somehow this works
if (inherits(args[[1]], c("ArrowObject", "array_expression"))) {
- # 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]]))
Review comment:
Alright, note that `negate_checked` does not support unsigned arguments:
```r
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])
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10196: ARROW-12575: [R] Use unary negative kernel
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10196:
URL: https://github.com/apache/arrow/pull/10196#issuecomment-829135758
https://issues.apache.org/jira/browse/ARROW-12575
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson closed pull request #10196: ARROW-12575: [R] Use unary negative kernel
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #10196:
URL: https://github.com/apache/arrow/pull/10196
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #10196: ARROW-12575: [R] Use unary negative kernel
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #10196:
URL: https://github.com/apache/arrow/pull/10196#issuecomment-832239849
> Wouldn't this just be testing R's parser?
In theory yes? I tend to have this philosophy re tests: https://twitter.com/gshotwell/status/1389563734679605251?s=20
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] thisisnic commented on a change in pull request #10196: ARROW-12575: [R] Use unary negative kernel
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10196:
URL: https://github.com/apache/arrow/pull/10196#discussion_r625950706
##########
File path: r/R/expression.R
##########
@@ -56,13 +56,9 @@ build_array_expression <- function(FUN,
args = list(...),
options = empty_named_list()) {
if (FUN == "-" && length(args) == 1L) {
- # Unary -, i.e. just make it negative, and somehow this works
if (inherits(args[[1]], c("ArrowObject", "array_expression"))) {
- # 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]]))
Review comment:
Perhaps for the sake of consistency, using `negate_checked` makes sense, even if not strictly necessary?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] edponce commented on a change in pull request #10196: ARROW-12575: [R] Use unary negative kernel
Posted by GitBox <gi...@apache.org>.
edponce commented on a change in pull request #10196:
URL: https://github.com/apache/arrow/pull/10196#discussion_r626084087
##########
File path: r/R/expression.R
##########
@@ -56,13 +56,9 @@ build_array_expression <- function(FUN,
args = list(...),
options = empty_named_list()) {
if (FUN == "-" && length(args) == 1L) {
- # Unary -, i.e. just make it negative, and somehow this works
if (inherits(args[[1]], c("ArrowObject", "array_expression"))) {
- # 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]]))
Review 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
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #10196: ARROW-12575: [R] Use unary negative kernel
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #10196:
URL: https://github.com/apache/arrow/pull/10196#issuecomment-832227640
> @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.
>
> ```
> ---2
> -(-(-2))
> --2
> -(-(2))
> ```
Wouldn't this just be testing R's parser?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org