Skip to content

ARROW-13280: [R] Bindings for log and trig functions - #10689

Closed
thisisnic wants to merge 10 commits into
apache:masterfrom
thisisnic:ARROW-13280-log
Closed

ARROW-13280: [R] Bindings for log and trig functions#10689
thisisnic wants to merge 10 commits into
apache:masterfrom
thisisnic:ARROW-13280-log

Conversation

@thisisnic

@thisisnicthisisnic commented Jul 9, 2021

Copy link
Copy Markdown
Member

No description provided.

@github-actions

github-actionsBot commented Jul 9, 2021

Copy link
Copy Markdown

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

@thisisnic

thisisnic commented Jul 9, 2021

Copy link
Copy Markdown
MemberAuthor

This PR will need rebasing off #10686 when it's merged

@lidavidm

lidavidm commented Jul 12, 2021

Copy link
Copy Markdown
Member

Sorry for the delay, I've just merged #10686

@thisisnicthisisnic changed the title ARROW-13280: [R] Bindings for log and trig functions [WIP]ARROW-13280: [R] Bindings for log and trig functionsJul 14, 2021
@thisisnic

thisisnic commented Jul 14, 2021

Copy link
Copy Markdown
MemberAuthor

Sorry for the delay, I've just merged #10686

All good, wasn't urgent! :)

nealrichardson
nealrichardson requested changes Jul 14, 2021
View reviewed changes

@nealrichardsonnealrichardson 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.

Looks good, just one request.

Comment threadr/R/expression.R Outdated

@nealrichardsonnealrichardsonJul 14, 2021

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.

Now that these exist, we should define the Math group generic for Arrow objects, like we have for Ops. I made https://issues.apache.org/jira/browse/ARROW-13337 for that.

@nealrichardsonnealrichardsonJul 14, 2021

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.

Hmm, in R, log is not unary:

log <- function (x, base = exp(1))

So we should handle that correctly. If arrow doesn't support general log_baseN, (1) make sure there's a JIRA for that, and (2) map the cases that do exist (e, 10, 2, etc.) and for now error "not supported" for any other value of base.

@thisisnicthisisnicJul 15, 2021

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.

Done and opened https://issues.apache.org/jira/browse/ARROW-13345

nealrichardson
nealrichardson reviewed Jul 15, 2021
View reviewed changes
Comment threadr/R/dplyr-functions.R Outdated
@asfimportasfimport mentioned this pull request Aug 13, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nealrichardsonnealrichardsonnealrichardson approved these changes

Assignees

No one assigned

Labels

Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@thisisnic@lidavidm@nealrichardson