ARROW-13448: [R] Bindings for strftime #11105

rok · 2021-09-07T20:13:45Z

This is to resolve ARROW-13448.

github-actions · 2021-09-07T20:14:05Z

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

github-actions · 2021-09-07T20:14:07Z

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

r/R/dplyr-functions.R

r/tests/testthat/test-dplyr-string-functions.R

r/src/compute.cpp

rok · 2021-09-16T19:15:35Z

Thanks for the review @thisisnic!

rok · 2021-09-16T19:28:45Z

Another thing to note here is that behavior of the %S flag is different in arrow:

Output precision of %S (seconds) flag depends on the input timestamp precision. Timestamps with second precision are represented as integers while milliseconds, microsecond and nanoseconds are represented as fixed floating point numbers with 3, 6 and 9 decimal places respectively. To obtain integer seconds, cast to timestamp with second resolution. The character for the decimal point is localized according to the locale. See detailed formatting documentation for descriptions of other flags.

This is already tested in c++ and we test it in Python like so. I'm not sure there's a point in testing it in R but it could look like this:

  arr <- Array$create(c(lubridate::ymd_hms("2018-10-07 19:04:05"), NA), type = timestamp("s"))
  expect_dplyr_equal(
    input %>%
      mutate(x = strftime(x, format = "%S")) %>%
      collect(),
    arr
  )

Except this wouldn't work for type reasons:

Error (test-dplyr-string-functions.R:756:3): strftime
Error: no applicable method for 'mutate' applied to an object of class "c('Array', 'ArrowDatum', 'ArrowObject', 'R6')"

thisisnic

Please can you also run the styler, so that the linter stops complaining?

r/tests/testthat/test-dplyr-string-functions.R

r/src/compute.cpp

thisisnic · 2021-09-16T21:51:31Z

behavior of the %S flag is different in arrow

So, the R "%S" returns an integer, so you're right in highlighting the difference between Arrow and R. We have other time-related functions which return slightly different values, which we allow for, e.g.

arrow/r/tests/testthat/test-dplyr-lubridate.R

Lines 155 to 164 in 5260fd5

    
           test_that("extract second from timestamp", { 
        
             expect_dplyr_equal( 
        
               input %>% 
        
                 mutate(x = second(datetime)) %>% 
        
                 collect(), 
        
               test_df, 
        
               # arrow supports nanosecond resolution but lubridate does not 
        
               tolerance = 1e-6 
        
             ) 
        
           })

The function expect_dplyr_equal(), as you've pointed out, won't work on Array objects. However, you could write a test like the one shown above, which just adds a tolerance (given that the user is being returned a more precise answer than they would have, the tolerance is justified).

I suppose the difficulty lies in the fact that the output is a string not a number, but there will be workarounds.

r/tests/testthat/test-dplyr-string-functions.R

r/R/dplyr-functions.R

Co-authored-by: Nic <thisisnic@gmail.com>

rok · 2021-09-18T22:47:09Z

r/tests/testthat/test-dplyr-string-functions.R

+  expect_dplyr_equal(
+    input %>%
+      mutate(x = strftime(x, format = formats_minus_c, tz = "Pacific/Marquesas")) %>%
+      collect(),
+    times
+  )
+
+  withr::with_locale(new = c("LC_TIME" = "C"),
+    expect_dplyr_equal(
+      input %>%
+        mutate(x = strftime(x, format = "%c", tz = "Pacific/Marquesas")) %>%
+        collect(),
+      times
+    )
+  )


I can't quite explain why, but it happens on my laptop. Could it be that strftime is using locale different from the one we use here (Sys.getlocale("LC_TIME"))?

It appears R's strftime interprets %c differently as date.h we use in arrow in certain locales and timezones?
Sometimes it's "%a %b %e %H:%M:%S %Y" and sometimes "%a %b %e %H:%M:%S %Y %Z".
Could also just be my laptop.

I'm a bit confused here - can you elaborate a bit more on your comments above please?

Sorry, it was late :).
These tests are failing locally for me on the %c flag in Pacific/Marquesas, but not for say UTC. I am not sure why - it could be a difference between the definition of %c in date.h we're using and in R's strftime. Or it could just be some setting on my laptop. Either way it's very strange to see an error on only this flag with this locale and timezone.

I'm more and more suspicious of my laptop really.

Another possibility is that the timezone database your system uses that may/may not be quite the same in different environments (cf https://stat.ethz.ch/R-manual/R-devel/library/base/html/timezones.html)

Could be. FWIW I'm on Ubuntu 21.04 and have LC_TIME=nl_NL.UTF-8 and Sys.timezone()-> "Europe/Amsterdam".
I'll remove the check and see what happens in the CI.
Should we add checks in for different locales into the tests?

If we're worried that there might be differences / bugs not respecting locales, having something non-default using withr::with_locale (like you do above) would be a good idea.

We do hardcode this for at least some of our CI builds

Ok, I think I finally understand what the issue is here. Arrow's strftime behaves differently than R's.
Arrow strftime(x, format = "%c %Z"):
Sat 01 Jan 2022 00:00:00 AM CET EST

R strftime(x, format = "%c"):
Sat 01 Jan 2022 12:00:00 AM EST EST

It's probably a bug in the C++ library we use so I opened an issue and added a reference to it in the comment HowardHinnant/date#704

I've disabled %c in R for now and I propose we merge this and revisit later.
@pitrou shall we block %c in c++ as well? See HowardHinnant/date#704

r/R/dplyr-functions.R

r/tests/testthat/test-dplyr-string-functions.R

rok · 2021-09-21T17:19:55Z

@thisisnic @jonkeane sorry this took a while. Could you please do another pass?

thisisnic

Looks good to me, but please can you give it a final check @jonkeane in case I've missed anything?

One other thing to note @rok - there was a comment on a duplicate ticket of this one which mentions implementing the lubridate package's format_ISO8601 function (see https://lubridate.tidyverse.org/reference/format_ISO8601.html). Please could you either:

implement that here, or
create a new ticket to implement it (and as you've done most of the hard work here, it could be a good candidate to label as good-first-issue or good-second-issue)

rok · 2021-09-22T11:05:35Z

@thisisnic good catch on format_ISO8601. If you don't have a strong preference I'll go ahead and add it.

Yet another thing to note: the c++ strfkernel only works for timestamps at the moment. When dates and times are added (ARROW-13916) we should add tests in R as well. I'll make a note on Jira.

rok · 2021-09-22T19:56:02Z

Ok, added format_ISO8601 and moved %c check into c++. I suppose another review is in order @thisisnic :)

r/R/dplyr-functions.R

Co-authored-by: Jonathan Keane <jkeane@gmail.com>

thisisnic

A couple of minor things, otherwise looking good!

r/R/dplyr-functions.R

Co-authored-by: Nic <thisisnic@gmail.com>

thisisnic

Thanks for implementing format_ISO8601 as well!

jonkeane

Looks good, thanks for this

rok · 2021-09-23T18:42:54Z

Thanks @thisisnic @jonkeane @edponce !

This is to resolve [ARROW-13448](https://issues.apache.org/jira/browse/ARROW-13448). Closes apache#11105 from rok/ARROW-13448 Lead-authored-by: Rok <rok@mihevc.org> Co-authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Jonathan Keane <jkeane@gmail.com>

github-actions bot added the Component: R label Sep 7, 2021

rok force-pushed the ARROW-13448 branch 2 times, most recently from 5568c27 to 8ad7ce3 Compare September 13, 2021 16:14

rok marked this pull request as ready for review September 13, 2021 18:48

rok force-pushed the ARROW-13448 branch from 8ad7ce3 to c272d2e Compare September 14, 2021 08:11

nealrichardson requested a review from thisisnic September 15, 2021 14:42

thisisnic requested changes Sep 16, 2021

View reviewed changes

r/R/dplyr-functions.R Outdated Show resolved Hide resolved

r/tests/testthat/test-dplyr-string-functions.R Outdated Show resolved Hide resolved

r/tests/testthat/test-dplyr-string-functions.R Outdated Show resolved Hide resolved

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

rok added 2 commits September 16, 2021 21:14

Adding StrftimeOptions.

588304e

Review feedback implementation.

463f142

rok force-pushed the ARROW-13448 branch from c272d2e to 463f142 Compare September 16, 2021 19:15

github-actions bot added the Component: Python label Sep 16, 2021

thisisnic requested changes Sep 16, 2021

View reviewed changes

r/tests/testthat/test-dplyr-string-functions.R Outdated Show resolved Hide resolved

r/src/compute.cpp Show resolved Hide resolved

edponce reviewed Sep 17, 2021

View reviewed changes

r/tests/testthat/test-dplyr-string-functions.R Outdated Show resolved Hide resolved

rok force-pushed the ARROW-13448 branch from f1f126a to 5b616b8 Compare September 17, 2021 09:22

edponce reviewed Sep 17, 2021

View reviewed changes

r/R/dplyr-functions.R Outdated Show resolved Hide resolved

rok commented Sep 17, 2021

View reviewed changes

r/R/dplyr-functions.R Outdated Show resolved Hide resolved

rok force-pushed the ARROW-13448 branch 5 times, most recently from 768a940 to ba9fe66 Compare September 18, 2021 22:15

Update r/tests/testthat/test-dplyr-string-functions.R

4678968

Co-authored-by: Nic <thisisnic@gmail.com>

rok force-pushed the ARROW-13448 branch from ba9fe66 to 4678968 Compare September 18, 2021 22:42

rok commented Sep 18, 2021

View reviewed changes

rok requested a review from thisisnic September 18, 2021 23:15

thisisnic requested a review from jonkeane September 20, 2021 12:28

thisisnic reviewed Sep 20, 2021

View reviewed changes

r/R/dplyr-functions.R Outdated Show resolved Hide resolved

jonkeane reviewed Sep 20, 2021

View reviewed changes

r/tests/testthat/test-dplyr-string-functions.R Outdated Show resolved Hide resolved

rok added 2 commits September 20, 2021 18:38

Review feedback.

a41a7d5

Disabling %c flag.

9e633c7

thisisnic requested a review from jonkeane September 22, 2021 10:21

thisisnic approved these changes Sep 22, 2021

View reviewed changes

github-actions bot added the Component: C++ label Sep 22, 2021

Adding format_ISO8601, movint %c check to c++.

6af5ad4

rok force-pushed the ARROW-13448 branch from 2e04fef to 6af5ad4 Compare September 22, 2021 20:00

jonkeane reviewed Sep 22, 2021

View reviewed changes

r/R/dplyr-functions.R Outdated Show resolved Hide resolved

Update r/R/dplyr-functions.R

3a74ae6

Co-authored-by: Jonathan Keane <jkeane@gmail.com>

thisisnic requested changes Sep 23, 2021

View reviewed changes

r/R/dplyr-functions.R Outdated Show resolved Hide resolved

r/R/dplyr-functions.R Outdated Show resolved Hide resolved

rok force-pushed the ARROW-13448 branch from 65aaf62 to 3c2e7ed Compare September 23, 2021 12:02

Update r/R/dplyr-functions.R

d45cd22

Co-authored-by: Nic <thisisnic@gmail.com>

rok force-pushed the ARROW-13448 branch from 3c2e7ed to d45cd22 Compare September 23, 2021 12:40

thisisnic approved these changes Sep 23, 2021

View reviewed changes

rok requested a review from jonkeane September 23, 2021 16:03

jonkeane approved these changes Sep 23, 2021

View reviewed changes

jonkeane closed this in 953ac8a Sep 23, 2021

jonkeane mentioned this pull request Sep 24, 2021

ARROW-12871: [R] upgrade to testthat 3e #11208

Closed

asfimport mentioned this pull request Sep 23, 2021

[R] Bindings for strftime #18755

Closed

ARROW-13448: [R] Bindings for strftime #11105

ARROW-13448: [R] Bindings for strftime #11105

Uh oh!

Conversation

rok commented Sep 7, 2021

Uh oh!

github-actions bot commented Sep 7, 2021

Uh oh!

github-actions bot commented Sep 7, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rok commented Sep 16, 2021

Uh oh!

rok commented Sep 16, 2021

Uh oh!

thisisnic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thisisnic commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

rok commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thisisnic left a comment

Choose a reason for hiding this comment

Uh oh!

rok commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rok commented Sep 22, 2021

Uh oh!

Uh oh!

thisisnic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thisisnic 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!

rok commented Sep 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

thisisnic commented Sep 16, 2021 •

edited

Loading

rok commented Sep 21, 2021 •

edited

Loading

rok commented Sep 22, 2021 •

edited

Loading