thisisnic commented on a change in pull request #11105:
URL: https://github.com/apache/arrow/pull/11105#discussion_r710319132
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -719,6 +719,55 @@ test_that("errors in strptime", {
)
})
+test_that("strftime", {
+ # TODO: consider reevaluating this workaround after ARROW-12980
+ withr::local_timezone("UTC")
+ t_stamp <- tibble(x = c(lubridate::ymd_hms("2018-10-07 19:04:05"), NA))
+ t_string <- tibble(x = c("2018-10-07 19:04:05.000000", NA))
+ t_string_2 <- tibble(x = c("07 October 2018 19:04:05.000000 UTC", NA))
+
+ # TODO: remove once we have tz support on windows ARROW-13168
+ if (tolower(Sys.info()[["sysname"]]) != "windows") {
+ expect_equal(
Review comment:
We've got a custom function `expect_dplyr_equal()`, which we tend to use
with a lot of these functions - they make it easier to ensure that the
behaviour of the implemented Arrow function matches that of the equivalent base
R or tidyverse function. Would you mind updating these tests to use that
helper function?
##########
File path: r/src/compute.cpp
##########
@@ -325,6 +325,19 @@ std::shared_ptr<arrow::compute::FunctionOptions>
make_compute_options(
cpp11::as_cpp<arrow::TimeUnit::type>(options["unit"]));
}
+ if (func_name == "strftime") {
+ using Options = arrow::compute::StrftimeOptions;
+ std::string format = "%Y-%m-%dT%H:%M:%S";
Review comment:
As there are no default values for `format` in the C++ code, you
probably don't want to set this here - the R code should either supply a value
for `format` or this should fail. See the way that the options are implemented
for the `extract_regex` kernel (in this file) as an example of this.
##########
File path: r/R/dplyr-functions.R
##########
@@ -672,6 +672,10 @@ nse_funcs$strptime <- function(x, format = "%Y-%m-%d
%H:%M:%S", tz = NULL, unit
Expression$create("strptime", x, options = list(format = format, unit =
unit))
}
+nse_funcs$strftime <- function(x, format = "%Y-%m-%d %H:%M:%S", locale="C") {
Review comment:
In the R bindings for these functions, the arguments and values should
match the base R or tidyverse function that is being implemented. This doesn't
quite match - here are the docs for base R's `strftime()`:
https://stat.ethz.ch/R-manual/R-devel/library/base/html/strptime.html
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -719,6 +719,55 @@ test_that("errors in strptime", {
)
})
+test_that("strftime", {
+ # TODO: consider reevaluating this workaround after ARROW-12980
+ withr::local_timezone("UTC")
+ t_stamp <- tibble(x = c(lubridate::ymd_hms("2018-10-07 19:04:05"), NA))
+ t_string <- tibble(x = c("2018-10-07 19:04:05.000000", NA))
+ t_string_2 <- tibble(x = c("07 October 2018 19:04:05.000000 UTC", NA))
+
+ # TODO: remove once we have tz support on windows ARROW-13168
+ if (tolower(Sys.info()[["sysname"]]) != "windows") {
+ expect_equal(
+ t_stamp %>%
+ Table$create() %>%
+ mutate(
+ x = strftime(x)
+ ) %>%
+ collect(),
+ t_string
+ )
+
+ expect_equal(
+ t_stamp %>%
+ Table$create() %>%
+ mutate(
+ x = strftime(x, format = "%Y-%m-%d %H:%M:%S")
+ ) %>%
+ collect(),
+ t_string
+ )
+
+ expect_equal(
+ t_stamp %>%
+ Table$create() %>%
+ mutate(
+ x = strftime(x, format = "%d %B %Y %H:%M:%S %Z", locale = "C")
+ ) %>%
+ collect(),
+ t_string_2
+ )
+ } else {
+ expect_error(
+ t_stamp %>%
+ Table$create() %>%
+ mutate(x = strftime(x)) %>%
+ collect(),
+ "Strftime not yet implemented on windows."
+ )
+ }
Review comment:
Please can you refactor this block to, instead of using a conditional,
use the `skip()` function. There should be some examples in other test files
you can follow, but let me know if it's not clear :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
