ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() #11850

paleolimbot · 2021-12-03T18:30:32Z

This PR implements the Duration type in the R bindings, mapped to "difftime", which is the equivalent base R type. Currently a draft PR because I'm unsure how to implement the Extend() method of the converter:

arrow/r/src/r_to_arrow.cpp

Lines 824 to 826 in 7240420

    
           Status Extend(SEXP x, int64_t size, int64_t offset = 0) override { 
        
             // TODO: look in lubridate 
        
             return Status::NotImplemented("Extend");

I've also copy-and-pasted the converter for the TIME32/hms object conversion because I was unsure how to subclass it to avoid the repetition in the moment.

The gist of the PR is to allow this (which currently fails):

library(arrow, warn.conflicts = FALSE)
Array$create(as.difftime(123, units = "secs"))
#> Error: NotImplemented: Extend

^{Created on 2021-12-03 by the reprex package (v2.0.1)}

github-actions · 2021-12-03T18:30:52Z

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

github-actions · 2021-12-03T18:30:53Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

Add ::Extend() method

r/src/r_to_arrow.cpp

Co-authored-by: Romain François <romain@rstudio.com>

r/src/r_to_arrow.cpp

paleolimbot · 2021-12-10T16:08:35Z

Ok! This now works:

library(arrow, warn.conflicts = FALSE)
(arr <- Array$create(as.difftime(123, units = "secs")))
#> Array
#> <time32[s]>
#> [
#>   00:02:03
#> ]
as.vector(arr)
#> Time difference of 123 secs

^{Created on 2021-12-10 by the reprex package (v2.0.1)}

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

paleolimbot · 2021-12-13T19:02:55Z

Just updating my reprex to show that it does, in fact work! (forgot to reinstall arrow from this branch before running reprex):

# remotes::insatll_github("paleolimbot/arrow/r@r-duration-type")
library(arrow, warn.conflicts = FALSE)
(arr <- Array$create(as.difftime(123, units = "secs")))
#> Array
#> <duration[s]>
#> [
#>   123
#> ]
as.vector(arr)
#> Time difference of 123 secs

nealrichardson · 2021-12-13T19:23:48Z

Thanks for this! Looks like there's one more entry in arrow.Rmd that needs to be edited, showing the mapping of Arrow Duration type to R, the table below the one that was.

jonkeane · 2021-12-13T19:28:56Z

Thanks for catching that. I've made https://issues.apache.org/jira/browse/ARROW-15082

ursabot · 2021-12-13T20:41:19Z

Benchmark runs are scheduled for baseline = ab46725 and contender = cf8d81d. cf8d81d 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 ⬇️0.45% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

thisisnic · 2021-12-14T13:44:46Z

I haven't implemented the lubridate::duration() translation because it seems out of scope for this set of changes but could also include it with this PR.

All good, though would you mind opening a new ticket for that?

paleolimbot · 2021-12-14T13:52:37Z

Done! See ARROW-15098.

@jonkeane

This PR is an attempt to fix the [sanitizer failure](https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=17171&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=d9b15392-e4ce-5e4c-0c8c-b69645229181&l=12824) caused by ARROW-14941 (#11850). @jonkeane could you remind me how to submit a job to the sanitizer checker? Closes #11947 from paleolimbot/r-fix-duration Authored-by: Dewey Dunnington <dewey@fishandwhistle.net> Signed-off-by: Antoine Pitrou <antoine@python.org>

The PR updates the 'Get Started' vignette that should have been updated to reflect the mapping of the duration type to R's difftime in #11850 / ARROW-14941 but wasn't. Closes #11948 from paleolimbot/r-duration-in-vignette Authored-by: Dewey Dunnington <dewey@fishandwhistle.net> Signed-off-by: Jonathan Keane <jkeane@gmail.com>

paleolimbot added 5 commits December 3, 2021 13:37

add duration type

3d0ad95

test more common case for duration unit

59bdaa1

add some C++ infrastructue for conversion

8f5739d

add failing test for difftime roundtrip

e49c6ab

fix table to update type for difftime and add hms

642306b

github-actions bot added the Component: R label Dec 3, 2021

romainfrancois self-requested a review December 6, 2021 10:11

paleolimbot and others added 5 commits December 9, 2021 09:07

add hms package name to hms class

5d54c6e

RPrimitiveConverter<duration>::Extend()

279bc78

comment

bcb1ce9

Merge pull request #1 from romainfrancois/r-duration-type-Extend

0113fc0

Add ::Extend() method

run clang-format

3073ad1

romainfrancois reviewed Dec 10, 2021

View reviewed changes

r/src/r_to_arrow.cpp Outdated Show resolved Hide resolved

Update r/src/r_to_arrow.cpp

32623e3

Co-authored-by: Romain François <romain@rstudio.com>

romainfrancois reviewed Dec 10, 2021

View reviewed changes

r/src/r_to_arrow.cpp Outdated Show resolved Hide resolved

paleolimbot added 2 commits December 10, 2021 11:49

remove comment

6789f35

more tests

45024c5

paleolimbot marked this pull request as ready for review December 10, 2021 16:06

romainfrancois approved these changes Dec 13, 2021

View reviewed changes

jonkeane closed this in cf8d81d Dec 13, 2021

jonkeane mentioned this pull request Dec 14, 2021

ARROW-14510: [R] [CI] ensure that docker runs don't use host-built artifacts #11944

Closed

This was referenced Dec 14, 2021

ARROW-15096: [R] Sanitizer failures with duration type #11947

Closed

ARROW-15082: [R] Clean up one more duration mapping entry #11948

Closed

paleolimbot deleted the r-duration-type branch December 9, 2022 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() #11850

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() #11850

Uh oh!

paleolimbot commented Dec 3, 2021

Uh oh!

github-actions bot commented Dec 3, 2021

Uh oh!

github-actions bot commented Dec 3, 2021

Uh oh!

Uh oh!

Uh oh!

paleolimbot commented Dec 10, 2021

Uh oh!

paleolimbot commented Dec 13, 2021

Uh oh!

nealrichardson commented Dec 13, 2021 •

edited

Loading

Uh oh!

jonkeane commented Dec 13, 2021

Uh oh!

ursabot commented Dec 13, 2021 •

edited

Loading

Uh oh!

thisisnic commented Dec 14, 2021

Uh oh!

paleolimbot commented Dec 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
	// TODO: look in lubridate
	return Status::NotImplemented("Extend");

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() #11850

ARROW-14941 [R] Implement Duration R6 class and bindings for lubridate::duration() #11850

Uh oh!

Conversation

paleolimbot commented Dec 3, 2021

Uh oh!

github-actions bot commented Dec 3, 2021

Uh oh!

github-actions bot commented Dec 3, 2021

Uh oh!

Uh oh!

Uh oh!

paleolimbot commented Dec 10, 2021

Uh oh!

paleolimbot commented Dec 13, 2021

Uh oh!

nealrichardson commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonkeane commented Dec 13, 2021

Uh oh!

ursabot commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thisisnic commented Dec 14, 2021

Uh oh!

paleolimbot commented Dec 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nealrichardson commented Dec 13, 2021 •

edited

Loading

ursabot commented Dec 13, 2021 •

edited

Loading