ARROW-13448: [R] Bindings for strftime

[rok]

This is to resolve ARROW-13448.

[github-actions]

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

[github-actions]

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

[rok]

Thanks for the review @thisisnic!

[rok]

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):strftimeError:noapplicablemethodfor'mutate'appliedtoanobjectofclass"c('Array', 'ArrowDatum', 'ArrowObject', 'R6')"

[thisisnic]

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

[thisisnic]

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.

[rok]

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"))?

[rok]

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.

[thisisnic]

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

[rok]

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.

[jonkeane]

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)

[rok]

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?

[jonkeane]

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

[rok]

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

[rok]

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

[rok]

@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:

1. implement that here, or
2. 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]

@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]

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

[thisisnic]

A couple of minor things, otherwise looking good!

[thisisnic]

Thanks for implementing format_ISO8601 as well!

[jonkeane]

Looks good, thanks for this

[rok]

Thanks @thisisnic@jonkeane@edponce !

[rok]

This is to resolve ARROW-13448.

[github-actions]

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

[github-actions]

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

[rok]

Thanks for the review @thisisnic!

[rok]

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):strftimeError:noapplicablemethodfor'mutate'appliedtoanobjectofclass"c('Array', 'ArrowDatum', 'ArrowObject', 'R6')"

[thisisnic]

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

[thisisnic]

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.

[rok]

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"))?

[rok]

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.

[thisisnic]

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

[rok]

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.

[jonkeane]

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)

[rok]

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?

[jonkeane]

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

[rok]

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

[rok]

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

[rok]

@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:

1. implement that here, or
2. 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]

@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]

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

[thisisnic]

A couple of minor things, otherwise looking good!

[thisisnic]

Thanks for implementing format_ISO8601 as well!

[jonkeane]

Looks good, thanks for this

[rok]

Thanks @thisisnic@jonkeane@edponce !

[rok]

This is to resolve ARROW-13448.

[github-actions]

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

[github-actions]

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

[rok]

Thanks for the review @thisisnic!

[rok]

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):strftimeError:noapplicablemethodfor'mutate'appliedtoanobjectofclass"c('Array', 'ArrowDatum', 'ArrowObject', 'R6')"

[thisisnic]

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

[thisisnic]

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.

[rok]

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"))?

[rok]

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.

[thisisnic]

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

[rok]

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.

[jonkeane]

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)

[rok]

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?

[jonkeane]

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

[rok]

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

[rok]

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

[rok]

@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:

1. implement that here, or
2. 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]

@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]

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

[thisisnic]

A couple of minor things, otherwise looking good!

[thisisnic]

Thanks for implementing format_ISO8601 as well!

[jonkeane]

Looks good, thanks for this

[rok]

Thanks @thisisnic@jonkeane@edponce !

[rok]

This is to resolve ARROW-13448.

[github-actions]

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

[github-actions]

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

[rok]

Thanks for the review @thisisnic!

[rok]

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):strftimeError:noapplicablemethodfor'mutate'appliedtoanobjectofclass"c('Array', 'ArrowDatum', 'ArrowObject', 'R6')"

[thisisnic]

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

[thisisnic]

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.

[rok]

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"))?

[rok]

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.

[thisisnic]

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

[rok]

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.

[jonkeane]

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)

[rok]

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?

[jonkeane]

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

[rok]

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

[rok]

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

[rok]

@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:

1. implement that here, or
2. 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]

@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]

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

[thisisnic]

A couple of minor things, otherwise looking good!

[thisisnic]

Thanks for implementing format_ISO8601 as well!

[jonkeane]

Looks good, thanks for this

[rok]

Thanks @thisisnic@jonkeane@edponce !

[rok]

This is to resolve ARROW-13448.

[github-actions]

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

[github-actions]

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

[rok]

Thanks for the review @thisisnic!

[rok]

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):strftimeError:noapplicablemethodfor'mutate'appliedtoanobjectofclass"c('Array', 'ArrowDatum', 'ArrowObject', 'R6')"

[thisisnic]

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

[thisisnic]

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.

[rok]

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"))?

[rok]

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.

[thisisnic]

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

[rok]

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.

[jonkeane]

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)

[rok]

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?

[jonkeane]

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

[rok]

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

[rok]

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

[rok]

@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:

1. implement that here, or
2. 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]

@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]

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

[thisisnic]

A couple of minor things, otherwise looking good!

[thisisnic]

Thanks for implementing format_ISO8601 as well!

[jonkeane]

Looks good, thanks for this

[rok]

Thanks @thisisnic@jonkeane@edponce !

[rok]

This is to resolve ARROW-13448.

[github-actions]

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

[github-actions]

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

[rok]

Thanks for the review @thisisnic!

[rok]

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):strftimeError:noapplicablemethodfor'mutate'appliedtoanobjectofclass"c('Array', 'ArrowDatum', 'ArrowObject', 'R6')"

[thisisnic]

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

[thisisnic]

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.

[rok]

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"))?

[rok]

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.

[thisisnic]

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

[rok]

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.

[jonkeane]

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)

[rok]

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?

[jonkeane]

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

[rok]

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

[rok]

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

[rok]

@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:

1. implement that here, or
2. 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]

@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]

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

[thisisnic]

A couple of minor things, otherwise looking good!

[thisisnic]

Thanks for implementing format_ISO8601 as well!

[jonkeane]

Looks good, thanks for this

[rok]

Thanks @thisisnic@jonkeane@edponce !

[rok]

This is to resolve ARROW-13448.

[github-actions]

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

[github-actions]

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

[rok]

Thanks for the review @thisisnic!

[rok]

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):strftimeError:noapplicablemethodfor'mutate'appliedtoanobjectofclass"c('Array', 'ArrowDatum', 'ArrowObject', 'R6')"

[thisisnic]

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

[thisisnic]

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.

[rok]

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"))?

[rok]

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.

[thisisnic]

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

[rok]

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.

[jonkeane]

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)

[rok]

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?

[jonkeane]

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

[rok]

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

[rok]

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

[rok]

@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:

1. implement that here, or
2. 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]

@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]

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

[thisisnic]

A couple of minor things, otherwise looking good!

[thisisnic]

Thanks for implementing format_ISO8601 as well!

[jonkeane]

Looks good, thanks for this

[rok]

Thanks @thisisnic@jonkeane@edponce !

[rok]

This is to resolve ARROW-13448.

[github-actions]

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

[github-actions]

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

[rok]

Thanks for the review @thisisnic!

[rok]

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):strftimeError:noapplicablemethodfor'mutate'appliedtoanobjectofclass"c('Array', 'ArrowDatum', 'ArrowObject', 'R6')"

[thisisnic]

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

[thisisnic]

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.

[rok]

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"))?

[rok]

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.

[thisisnic]

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

[rok]

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.

[jonkeane]

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)

[rok]

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?

[jonkeane]

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

[rok]

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

[rok]

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

[rok]

@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:

1. implement that here, or
2. 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]

@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]

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

[thisisnic]

A couple of minor things, otherwise looking good!

[thisisnic]

Thanks for implementing format_ISO8601 as well!

[jonkeane]

Looks good, thanks for this

[rok]

Thanks @thisisnic@jonkeane@edponce !