Skip to content

ARROW-9187: [R] Add bindings for arithmetic kernels - #8947

Closed
nealrichardson wants to merge 33 commits into
apache:masterfrom
nealrichardson:arithmetic-expressions
Closed

ARROW-9187: [R] Add bindings for arithmetic kernels#8947
nealrichardson wants to merge 33 commits into
apache:masterfrom
nealrichardson:arithmetic-expressions

Conversation

@nealrichardson

@nealrichardsonnealrichardson commented Dec 17, 2020

Copy link
Copy Markdown
Member

Based on ARROW-10322

bkietzand others added 18 commits December 15, 2020 10:47
@bkietz@nealrichardson
ARROW-10322: [C++][Dataset] Minimize dataset::Expression
619fa13
@bkietz@nealrichardson
replace filtering with Expression2
1152d2c
@bkietz@nealrichardson
remove ExpressionState
d9c6ae5
@bkietz@nealrichardson
repair implicit casts
36f1bbb
@bkietz@nealrichardson
delete Expression DSL operators and old Expression class
b954ef8
@bkietz@nealrichardson
rename Expression2 -> Expression
e179f42
@bkietz@nealrichardson
first pass at repairing bindings
c3e6be3
@bkietz@nealrichardson
add more scalar cast implementations
27ebb46
@bkietz@nealrichardson
use dataset___expr__call to create Call expressions
9245f24
@bkietz@nealrichardson
lint fixes
7fc23dd
@nealrichardson
Refactor dataset expression code to follow array expression pattern
d9ec4bf
@nealrichardson
A little more
35b9e1f
@bkietz
revamp Expression::ToString, make cast-from-string error more informa…
ec6ee9f
…tive
@bkietz
Merge branch '10322-Minimize-Expression-to-a-' of github.com:bkietz/a…
c560cbb
…rrow into 10322-Minimize-Expression-to-a-
@bkietz
print and_kleene, or_kleene as binary ops
8bb9b90
@bkietz
clean up Expression class, extract Parameter
7c88e35
@bkietz
revert variant noexcept changes
dfad163
@nealrichardson
Start adding support for arithmetic functions in R
b32f254
@github-actions

github-actionsBot commented Dec 17, 2020

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-9187

@nealrichardson
nealrichardson marked this pull request as ready for review December 29, 2020 18:34
nealrichardson
nealrichardson commented Dec 29, 2020
View reviewed changes
Comment threadr/R/expression.R Outdated
Comment threadr/R/expression.R Outdated
Comment threadr/R/expression.R Outdated
Comment threadr/R/expression.R Outdated
Comment threadr/R/expression.R Outdated
Comment threadr/tests/testthat/test-dplyr.R Outdated
Comment threadr/tests/testthat/test-dplyr.R

expect_dplyr_equal(
input %>%
filter(dbl %/% 2 > 3) %>%

@nealrichardsonnealrichardsonDec 29, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these test are with dbl; should we add some with int or other columns? Or do you think the other types are better tested elsewhere (test-compute-arith.R) and this is really just testing the dplyr NSE?

Comment threadr/tests/testthat/test-compute-arith.R Outdated
Comment threadr/tests/testthat/test-compute-arith.R Outdated
nealrichardson
nealrichardson commented Dec 30, 2020
View reviewed changes
Comment threadr/R/expression.R Outdated
Comment threadr/R/expression.R Outdated
return(out)
}

# hack to use subtract instead of subtract_checked for timestamps

@nealrichardsonnealrichardsonDec 30, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only subtract?

And technically this could also be else if from above

Comment threadr/R/expression.R Outdated
Comment threadr/tests/testthat/test-compute-arith.R Outdated
b <- Scalar$create(Sys.time())
result <- a - b
expect_is(result$type, "DataType")
expect_identical(result$type$ToString(), "duration[us]")

@nealrichardsonnealrichardsonDec 30, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(1) I don't think this is right. timestamp + integer = timestamp, but because integer is cast to timestamp, you get duration.
(1b) Maybe you could support that by casting the integer to duration (same units as the timestamp), but this is a bunch of stuff that should probably get handled in C++
(2) According to https://arrow.apache.org/docs/r/articles/arrow.html#arrow-to-r, we don't support converting duration types to R.

Given this, I think arithmetic with dates/times/timestamps should be punted to its own JIRA.

@jonkeanejonkeaneDec 31, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've made ARROW-11090 — I'm honestly not sure these tests are even worth keeping + skipping with that since we will almost certainly make better support for durations (and other time-related types that don't have as much support) arrow > R at that point.

Comment threadr/tests/testthat/test-dataset.R Outdated
Comment threadr/tests/testthat/test-dplyr.R Outdated
Comment threadr/R/expression.R Outdated
@nealrichardsonnealrichardson mentioned this pull request Jan 6, 2021
Closed
@nealrichardson

nealrichardson commented Jan 6, 2021

Copy link
Copy Markdown
MemberAuthor

Rebase wasn't clean so I opened #9117 with the commits cherry-picked.

nealrichardson added a commit that referenced this pull request Jan 7, 2021
@jonkeane@nealrichardson
ARROW-9187: [R] Add bindings for arithmetic kernels
dfee391
Replaces #8947@jonkeaneCloses#9117 from nealrichardson/arith2
Lead-authored-by: Jonathan Keane <jkeane@gmail.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
@asfimportasfimport mentioned this pull request Jan 7, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jonkeanejonkeanejonkeane left review comments

Assignees

No one assigned

Labels

Component: C++Component: PythonComponent: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nealrichardson@jonkeane@bkietz