Skip to content

ARROW-16415: [R] Update strptime binding signature with the tz argument #13190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jonkeane merged 12 commits into from
Jun 7, 2022
Merged

ARROW-16415: [R] Update strptime binding signature with the tz argument #13190

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
aaa43a5
adding `tz` support to `strptime` + updated unit tests
dragosmg May 18, 2022
2619723
set default value for `tz` to match `base::strptime()`
dragosmg May 25, 2022
f9c29a1
update unit tests
dragosmg May 25, 2022
b42453e
bump ci
dragosmg May 25, 2022
c27dbf9
bump ci
dragosmg May 25, 2022
5525acc
bump ci
dragosmg May 25, 2022
e34f552
comment + `record_batch()` in one of the tests
dragosmg May 26, 2022
8e216aa
added comment on the use of `assume_timezone`
dragosmg Jun 6, 2022
ee73795
expanded on why `assume_timezone` is needed
dragosmg Jun 7, 2022
f0c3323
improved comments and renamed one of the test tibbles for more clarity
dragosmg Jun 7, 2022
ca4e902
forgot about one `t_stamp`
dragosmg Jun 7, 2022
90ce31b
Update r/tests/testthat/test-dplyr-funcs-datetime.R
jonkeane Jun 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 32 additions & 9 deletions r/R/dplyr-funcs-datetime.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,25 +28,48 @@ register_bindings_datetime <- function() {
}

register_bindings_datetime_utility <- function() {
register_binding("strptime", function(x, format = "%Y-%m-%d %H:%M:%S", tz = NULL,
register_binding("strptime", function(x,
format = "%Y-%m-%d %H:%M:%S",
tz = NULL,
unit = "ms") {
# Arrow uses unit for time parsing, strptime() does not.
# Arrow has no default option for strptime (format, unit),
# we suggest following format = "%Y-%m-%d %H:%M:%S", unit = MILLI/1L/"ms",
# (ARROW-12809)

# ParseTimestampStrptime currently ignores the timezone information (ARROW-12820).
# Stop if tz is provided.
if (is.character(tz)) {
arrow_not_supported("Time zone argument")
}
unit <- make_valid_time_unit(
unit,
c(valid_time64_units, valid_time32_units)
)

unit <- make_valid_time_unit(unit, c(valid_time64_units, valid_time32_units))
output <- build_expr(
"strptime",
x,
options =
list(
format = format,
unit = unit,
error_is_null = TRUE
)
)

build_expr("strptime", x, options = list(format = format, unit = unit, error_is_null = TRUE))
if (!is.null(tz)) {
output <- build_expr(
"assume_timezone",
output,
options =
list(
timezone = tz
)
)
}
output
Comment thread
dragosmg marked this conversation as resolved.
})

register_binding("strftime", function(x, format = "", tz = "", usetz = FALSE) {
register_binding("strftime", function(x,
format = "",
tz = "",
usetz = FALSE) {
if (usetz) {
format <- paste(format, "%Z")
}
Expand Down
22 changes: 13 additions & 9 deletions r/tests/testthat/test-dplyr-funcs-datetime.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,19 @@ test_df <- tibble::tibble(
test_that("strptime", {
t_string <- tibble(x = c("2018-10-07 19:04:05", NA))
t_stamp <- tibble(x = c(lubridate::ymd_hms("2018-10-07 19:04:05"), NA))
t_stamp_with_tz <- tibble(
x = c(lubridate::ymd_hms("2018-10-07 19:04:05", tz = "Pacific/Marquesas"), NA)
)

expect_equal(
t_string %>%
arrow_table() %>%
mutate(
x = strptime(x, format = "%Y-%m-%d %H:%M:%S", tz = "Pacific/Marquesas")
) %>%
collect(),
t_stamp_with_tz
)

expect_equal(
t_string %>%
Expand Down Expand Up @@ -111,15 +124,6 @@ test_that("strptime", {
)
})

test_that("errors in strptime", {
# Error when tz is passed
x <- Expression$field_ref("x")
expect_error(
call_binding("strptime", x, tz = "PDT"),
"Time zone argument not supported in Arrow"
)
})

test_that("strptime returns NA when format doesn't match the data", {
df <- tibble(
str_date = c("2022-02-07", "2012/02-07", "1975/01-02", "1981/01-07", NA)
Expand Down