ARROW-14942: [R] Bindings for lubridate's dpicoseconds, dnanoseconds, desconds, dmilliseconds, dmicroseconds by AlenkaF · Pull Request #12855 · apache/arrow

AlenkaF · 2022-04-11T10:23:40Z

This PR adds bindings for lubridate's dseconds, dmilliseconds, dmicroseconds and dnanoseconds.

As picoseconds are not supported by duration in Arrow and duration is of integer type, the call to picoseconds() raises a warning.

github-actions · 2022-04-11T10:25:39Z

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

amol- · 2022-04-13T10:17:27Z

@dragosmg mind reviewing this one?

dragosmg · 2022-04-14T08:31:23Z

I've been thinking a bit about this. Do you think it's worth having a helper function (to avoid all the repetition), something like make_duration(x, unit)?
Where:

make_duration<-function(x, unit) {
x<- build_expr("cast", x, options= cast_options(to_type= int64()))
x$cast(duration(unit))
}

AlenkaF · 2022-04-14T08:36:29Z

Sure, makes sense 👍 Will do.

dragosmg

Great work. Many thanks. @thisisnic @jonkeane would you mind taking a look and merging the PR.

jonkeane

This looks great, I have one substantive comment about test additions, one small suggestion, and a comment.

jonkeane · 2022-04-15T14:27:39Z

Should we also test what happens when we pass floats here too?
> lubridate::dseconds(1.5) [1] "1.5s"
Seems to work, so we should ensure we can do that (or error helpfully if we can't for some reason)

Of course, thanks for this!
Will search for discussions Dragos already had about casting float -> duration, then test and see =)

As duration type in Arrow is int64 and we can't pass floats here I will go with erroring helpfully. Will add it in the next commit.

ARROW-16253 might be relevant here too.

I added a test for the error when the argument multiplied with the value of the multiplication factor of the duration helper function is float (went with easier solution - didn't go forward with forcing evaluation to check for type of an argument or try catching C++ error).

…econds

AlenkaF · 2022-04-21T15:25:58Z

@jonkeane I tried to address all the comments and I think the PR is ready for another review.

AlenkaF · 2022-04-21T15:27:45Z

The errors do not look related ...

jonkeane

This is fantastic, thank you so much for the work on this.

I have one small question about possibly adding a comment — let me know if you want to add that and I'll wait to merge

jonkeane · 2022-04-21T18:27:52Z

+ duration_helpers_map_factory <- function(value, unit) {
+ force(value)
+ force(unit)
+ function(x = 1) make_duration(x * value, unit)
+ }
+
+ for (name in names(.helpers_function_map)) {
+ register_binding(
+ name,
+ duration_helpers_map_factory(
+ .helpers_function_map[[name]][[1]],
+ .helpers_function_map[[name]][[2]]
+ )
+ )
+ }


Nice! This is actually even shorter than I though it would be!

jonkeane · 2022-04-21T18:28:47Z

+ # double -> duration not supported in Arrow.
+ # Error is generated in the C++ code
+ expect_error(
+ test_df %>%
+ arrow_table() %>%
+ mutate(r_obj_dminutes = dminutes(1.12345)) %>%
+ collect()
+ )


Thanks for this comment about why we are expect_error() but not actually asserting it (since this is all C++). 💯

jonkeane · 2022-04-21T18:30:35Z

+ ) %>%
+ collect(),
+ example_d,
+ ignore_attr = TRUE


I didn't see this in the PR (though might have missed something), what attr are we ignoring? Maybe we should add a comment about what we're using that for

I will add a comment, you can wait with merging. But I have to remember, if I am honest =) Will do it tomorrow morning and add the comment then.
Thank you for the review!

We are using ignore_attr = TRUE due to the diff in attributes package, units and class: (difftime vs Duration). I added a comment about it in the beginning of both tests.

AlenkaF · 2022-04-22T06:10:34Z

Errors do not seem to be related to this PR.

ursabot · 2022-04-25T17:41:48Z

Benchmark runs are scheduled for baseline = 0ce8ce8 and contender = c4b646e. c4b646e is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed ⬇️1.13% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.25% ⬆️0.08%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/586|c4b646e7 ec2-t3-xlarge-us-east-2>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/574|c4b646e7 test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/572|c4b646e7 ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/584|c4b646e7 ursa-thinkcentre-m75q>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/585|0ce8ce8b ec2-t3-xlarge-us-east-2>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/573|0ce8ce8b test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/571|0ce8ce8b ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/583|0ce8ce8b ursa-thinkcentre-m75q>
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

ursabot · 2022-04-26T22:24:34Z

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

github-actionsBot added the Component: R label Apr 11, 2022

AlenkaF mentioned this pull request Apr 11, 2022
ARROW-14943: [R] Bindings for lubridate's ddays, dhours, dminutes, dmonths, dweeks, dyears #12610
Closed

dragosmg reviewed Apr 13, 2022
View reviewed changes

Comment threadr/R/dplyr-funcs-datetime.R Outdated

dragosmg reviewed Apr 13, 2022
View reviewed changes

Comment threadr/R/dplyr-funcs-datetime.R Outdated

dragosmg reviewed Apr 13, 2022
View reviewed changes

Comment threadr/R/dplyr-funcs-datetime.R Outdated

AlenkaF force-pushed the ARROW-14942 branch from 0ff7399 to b700150Compare April 14, 2022 08:27

dragosmg suggested changes Apr 14, 2022
View reviewed changes

Comment threadr/R/dplyr-funcs-datetime.R Outdated

dragosmg approved these changes Apr 14, 2022
View reviewed changes

thisisnic self-requested a review April 14, 2022 16:56

jonkeane requested changes Apr 15, 2022
View reviewed changes

AlenkaF force-pushed the ARROW-14942 branch from 5bf9a82 to ee18157Compare April 20, 2022 12:15

AlenkaF added 15 commits April 21, 2022 14:22

Add implementation for dseconds, dmilliseconds, dmicroseconds, dnanos…
df230a5
…econds

Correct test for dpicoseconds
1dc35c6

Add a check for argument not an Expression and amend the tests
0ca4d80

Move the duration helpers into register_bindings_duration_helpers
b7de259

Replace Expression() with build_expr()
5bd6a5e

Add a helper function to avoid repetition
d139be7

Make make_duration a standalone function
f0678b1

Correct two typos left from the conflict merge
26a30c5

testing
aa3d54f

Add implementation for dseconds, dmilliseconds, dmicroseconds, dnanos…
10ce36b
…econds

Correct test for dpicoseconds
9aca24d

Add a check for argument not an Expression and amend the tests
3991f5c

Move the duration helpers into register_bindings_duration_helpers
9a6c5f7

Replace Expression() with build_expr()
115c6c4

Add a helper function to avoid repetition
eda6b8c

AlenkaF added 5 commits April 21, 2022 14:24

Make make_duration a standalone function
13e11a7

testing
23ca370

Create map factory for duration helpers
bef9aa9

Redo tz change lost in a force-push
17b7fa4

Add a a test to check for error in case of a double
55ab2c7

AlenkaF force-pushed the ARROW-14942 branch from c161a6b to 55ab2c7Compare April 21, 2022 12:26

AlenkaF added 2 commits April 21, 2022 14:28

Correct content in NEWS.md
6e0c9bb

Fix typo
00a5eef

jonkeane mentioned this pull request Apr 21, 2022
ARROW-15800 [R] Implement bindings for lubridate::as_date() and lubridate::as_datetime()#12738
Closed

jonkeane approved these changes Apr 21, 2022
View reviewed changes

Add info about the use of ignore_attr = TRUE in the tests
cfae99c

AlenkaF requested a review from jonkeane April 22, 2022 06:10

jonkeane closed this in c4b646eApr 22, 2022

AlenkaF deleted the ARROW-14942 branch April 22, 2022 14:45

asfimport mentioned this pull request Apr 26, 2022
[R] Bindings for lubridate's dpicoseconds, dnanoseconds, desconds, dmilliseconds, dmicroseconds #18975
Closed

Uh oh!

Conversation

AlenkaF commented Apr 11, 2022

Uh oh!

github-actionsBot commented Apr 11, 2022

Uh oh!

amol- commented Apr 13, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dragosmg commented Apr 14, 2022

Uh oh!

AlenkaF commented Apr 14, 2022

Uh oh!

Uh oh!

dragosmg left a comment

Choose a reason for hiding this comment

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlenkaFApr 15, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlenkaF commented Apr 21, 2022

Uh oh!

AlenkaF commented Apr 21, 2022

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlenkaF commented Apr 22, 2022

Uh oh!

ursabot commented Apr 25, 2022

Uh oh!

ursabot commented Apr 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

amol- commented Apr 13, 2022•
edited
Loading

AlenkaFApr 15, 2022•
edited
Loading