You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/03/04 22:53:31 UTC
[GitHub] [arrow] jonkeane commented on a change in pull request #12319: ARROW-14199 [R] bindings for format (where possible)
jonkeane commented on a change in pull request #12319:
URL: https://github.com/apache/arrow/pull/12319#discussion_r819969875
##########
File path: r/tests/testthat/test-dplyr-funcs-type.R
##########
@@ -843,3 +843,90 @@ test_that("as.Date() converts successfully from date, timestamp, integer, char a
test_df
)
})
+
+test_that("format date/time", {
+ skip_on_os("windows") # https://issues.apache.org/jira/browse/ARROW-13168
+
+ times <- tibble(
+ datetime = c(lubridate::ymd_hms("2018-10-07 19:04:05", tz = "Pacific/Marquesas"), NA),
+ date = c(as.Date("2021-01-01"), NA)
+ )
+ formats <- "%a %A %w %d %b %B %m %y %Y %H %I %p %M %z %Z %j %U %W %x %X %% %G %V %u"
+ formats_date <- "%a %A %w %d %b %B %m %y %Y %H %I %p %M %j %U %W %x %X %% %G %V %u"
+
+ compare_dplyr_binding(
+ .input %>%
+ mutate(x = format(datetime, format = formats)) %>%
+ collect(),
+ times
+ )
+
+ compare_dplyr_binding(
+ .input %>%
+ mutate(x = format(date, format = formats_date)) %>%
+ collect(),
+ times
+ )
+
+ compare_dplyr_binding(
+ .input %>%
+ mutate(x = format(datetime, format = formats, tz = "Europe/Bucharest")) %>%
+ collect(),
+ times
+ )
+
+ compare_dplyr_binding(
+ .input %>%
+ mutate(x = format(datetime, format = formats, tz = "EST", usetz = TRUE)) %>%
+ collect(),
+ times
+ )
+
+ withr::with_timezone(
+ "Pacific/Marquesas",
+ {
+ compare_dplyr_binding(
+ .input %>%
+ mutate(
+ x = format(datetime, format = formats, tz = "EST"),
+ x_date = format(date, format = formats_date, tz = "EST")
+ ) %>%
+ collect(),
+ times
+ )
+
+ compare_dplyr_binding(
+ .input %>%
+ mutate(
+ x = format(datetime, format = formats),
+ x_date = format(date, format = formats_date)
+ ) %>%
+ collect(),
+ times
+ )
+ }
+ )
+})
Review comment:
It might be nice to also test `format(1)` or the like for if someone provides an R object in the pipeline.
##########
File path: r/tests/testthat/test-dplyr-funcs-type.R
##########
@@ -843,3 +843,90 @@ test_that("as.Date() converts successfully from date, timestamp, integer, char a
test_df
)
})
+
+test_that("format date/time", {
+ skip_on_os("windows") # https://issues.apache.org/jira/browse/ARROW-13168
+
+ times <- tibble(
+ datetime = c(lubridate::ymd_hms("2018-10-07 19:04:05", tz = "Pacific/Marquesas"), NA),
+ date = c(as.Date("2021-01-01"), NA)
+ )
+ formats <- "%a %A %w %d %b %B %m %y %Y %H %I %p %M %z %Z %j %U %W %x %X %% %G %V %u"
+ formats_date <- "%a %A %w %d %b %B %m %y %Y %H %I %p %M %j %U %W %x %X %% %G %V %u"
+
+ compare_dplyr_binding(
+ .input %>%
+ mutate(x = format(datetime, format = formats)) %>%
+ collect(),
+ times
+ )
+
+ compare_dplyr_binding(
+ .input %>%
+ mutate(x = format(date, format = formats_date)) %>%
+ collect(),
+ times
+ )
+
+ compare_dplyr_binding(
+ .input %>%
+ mutate(x = format(datetime, format = formats, tz = "Europe/Bucharest")) %>%
+ collect(),
+ times
+ )
+
+ compare_dplyr_binding(
+ .input %>%
+ mutate(x = format(datetime, format = formats, tz = "EST", usetz = TRUE)) %>%
+ collect(),
+ times
+ )
+
+ withr::with_timezone(
+ "Pacific/Marquesas",
+ {
+ compare_dplyr_binding(
+ .input %>%
+ mutate(
+ x = format(datetime, format = formats, tz = "EST"),
+ x_date = format(date, format = formats_date, tz = "EST")
+ ) %>%
+ collect(),
+ times
+ )
+
+ compare_dplyr_binding(
+ .input %>%
+ mutate(
+ x = format(datetime, format = formats),
+ x_date = format(date, format = formats_date)
+ ) %>%
+ collect(),
+ times
+ )
+ }
+ )
+})
+
+test_that("format() for unsupported types returns the input as string", {
+ expect_equal(
+ example_data %>%
+ record_batch() %>%
+ mutate(x = format(int, trim = TRUE)) %>%
+ collect(),
+ example_data %>%
+ record_batch() %>%
+ mutate(x = as.character(int)) %>%
+ collect()
+ )
+ expect_equal(
+ example_data %>%
+ record_batch() %>%
+ mutate(y = format(dbl, nsmall = 3)) %>%
+ collect(),
+ example_data %>%
+ record_batch() %>%
+ mutate(y = as.character(dbl)) %>%
+ collect()
+ )
+})
Review comment:
Could we have one of these two use `arrow_table()` instead of `record_batch()` just to confirm that path way works too?
##########
File path: r/R/dplyr-funcs-type.R
##########
@@ -292,3 +293,18 @@ register_bindings_type_elementwise <- function() {
is_inf & !call_binding("is.na", is_inf)
})
}
+
+register_bindings_type_format <- function() {
+ register_binding("format", function(x, ...) {
+ if (!inherits(x, "Expression")) {
+ return(format(x, ...))
+ }
+
+ if (inherits(x, "Expression") &&
+ x$type_id() %in% Type[c("TIMESTAMP", "DATE32", "DATE64")]) {
+ binding_format_datetime(x, ...)
+ } else {
+ x$cast(string())
Review comment:
Should we use `build_expr("cast", x, ...)` here (and then we can likely get rid of the bit about that returns early if x isn't an expression, I think...)?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org