Skip to content

ARROW-8938: [R] Provide binding for arrow::compute::CallFunction - #7279

Closed
nealrichardson wants to merge 7 commits into
apache:masterfrom
nealrichardson:r-call-function
Closed

ARROW-8938: [R] Provide binding for arrow::compute::CallFunction#7279
nealrichardson wants to merge 7 commits into
apache:masterfrom
nealrichardson:r-call-function

Conversation

@nealrichardson

@nealrichardsonnealrichardson commented May 26, 2020•
edited
Loading

Copy link
Copy Markdown
Member

No description provided.

@github-actions

github-actionsBot commented May 26, 2020

Copy link
Copy Markdown

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

nealrichardson
nealrichardson commented May 26, 2020
View reviewed changes
Comment threadr/R/array.R
i <- Array$create(i)
}
if (inherits(i, "ChunkedArray")) {
# Invalid: Kernel does not support chunked array arguments

@nealrichardsonnealrichardsonMay 26, 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.

@wesm I didn't see a JIRA for this but admittedly there are a lot attached to the umbrella ticket so I may have missed it. Is there one for supporting Take with ChunkedArrays (as either the first and/or second argument)? Likewise for Take/Filter on RecordBatch and Table.

@nealrichardsonnealrichardsonMay 28, 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.

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

Comment threadr/src/compute.cpp
Comment threadr/src/compute.cpp Outdated
Comment threadr/src/compute.cpp Outdated
@nealrichardson

nealrichardson commented May 27, 2020

Copy link
Copy Markdown
MemberAuthor

@github-actions autotune

wesm
wesm reviewed May 27, 2020
View reviewed changes

@wesmwesm left a comment

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.

Cool, this should be helpful to simplify exposing compute functionality

Comment threadr/src/compute.cpp
Comment threadr/src/compute.cpp Outdated
nealrichardsonand others added 2 commits May 27, 2020 09:44
@nealrichardson@bkietz
Update r/src/compute.cpp
3210659
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
@bkietz
add Scalar R6 wrapper
2a67b5d
nealrichardson
nealrichardson commented May 27, 2020
View reviewed changes

@nealrichardsonnealrichardson left a comment

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.

Thanks for doing this

Comment threadr/R/scalar.R Outdated
Comment threadr/src/expression.cpp Outdated
Comment threadr/R/scalar.R Outdated
Comment threadr/src/compute.cpp
wesm
wesm reviewed May 28, 2020
View reviewed changes

@wesmwesm left a comment

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.

Some small comments. This looks OK to me modulo CI

Comment threadr/R/scalar.R Outdated
Comment threadr/R/scalar.R Outdated
Comment threadr/src/compute.cpp

std::shared_ptr<arrow::compute::FunctionOptions> make_compute_options(
std::string func_name, List_ options) {
if (func_name == "filter") {

@wesmwesmMay 28, 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.

Nit: might want to put these function names in constants or defines (doesn't need to be done this patch)

nealrichardson
nealrichardson commented May 28, 2020
View reviewed changes
Comment threadr/src/scalar.cpp Outdated
#include <arrow/scalar.h>

// [[arrow::export]]
std::shared_ptr<arrow::Scalar> Array__GetScalar(const std::shared_ptr<arrow::Array>& x, int64_t i) {

@nealrichardsonnealrichardsonMay 28, 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.

I removed the bespoke Scalar__create logic in favor of reusing the Array logic, like we are doing on the scalar-to-vector side. Scalar$create creates a length-1 Array and takes the 0th element as a Scalar. This added support for nested types as Scalar for free, and prevents other inconsistencies from arising.

@nealrichardson
nealrichardson marked this pull request as ready for review May 28, 2020 21:02
@nealrichardson

nealrichardson commented May 28, 2020

Copy link
Copy Markdown
MemberAuthor

There's more to do but this is enough to provide a foundation for future compute work in R.

@wesm

wesm commented May 29, 2020

Copy link
Copy Markdown
Member

+1

@nealrichardson
nealrichardson deleted the r-call-function branch May 29, 2020 21:04
@asfimportasfimport mentioned this pull request May 29, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wesmwesmwesm left review comments
@bkietzbkietzbkietz left review comments
@romainfrancoisromainfrancoisAwaiting requested review from romainfrancois

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nealrichardson@wesm@bkietz