Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
df230a5
Add implementation for dseconds, dmilliseconds, dmicroseconds, dnanos…
AlenkaF Apr 11, 2022
1dc35c6
Correct test for dpicoseconds
AlenkaF Apr 12, 2022
0ca4d80
Add a check for argument not an Expression and amend the tests
AlenkaF Apr 12, 2022
b7de259
Move the duration helpers into register_bindings_duration_helpers
AlenkaF Apr 14, 2022
5bd6a5e
Replace Expression() with build_expr()
AlenkaF Apr 14, 2022
d139be7
Add a helper function to avoid repetition
AlenkaF Apr 14, 2022
f0678b1
Make make_duration a standalone function
AlenkaF Apr 14, 2022
26a30c5
Correct two typos left from the conflict merge
AlenkaF Apr 15, 2022
aa3d54f
testing
AlenkaF Apr 20, 2022
10ce36b
Add implementation for dseconds, dmilliseconds, dmicroseconds, dnanos…
AlenkaF Apr 11, 2022
9aca24d
Correct test for dpicoseconds
AlenkaF Apr 12, 2022
3991f5c
Add a check for argument not an Expression and amend the tests
AlenkaF Apr 12, 2022
9a6c5f7
Move the duration helpers into register_bindings_duration_helpers
AlenkaF Apr 14, 2022
115c6c4
Replace Expression() with build_expr()
AlenkaF Apr 14, 2022
eda6b8c
Add a helper function to avoid repetition
AlenkaF Apr 14, 2022
13e11a7
Make make_duration a standalone function
AlenkaF Apr 14, 2022
23ca370
testing
AlenkaF Apr 20, 2022
bef9aa9
Create map factory for duration helpers
AlenkaF Apr 20, 2022
17b7fa4
Redo tz change lost in a force-push
AlenkaF Apr 21, 2022
55ab2c7
Add a a test to check for error in case of a double
AlenkaF Apr 21, 2022
6e0c9bb
Correct content in NEWS.md
AlenkaF Apr 21, 2022
00a5eef
Fix typo
AlenkaF Apr 21, 2022
cfae99c
Add info about the use of ignore_attr = TRUE in the tests
AlenkaF Apr 22, 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
1 change: 1 addition & 0 deletions r/NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
* Added `make_date()` & `make_datetime()` + `ISOdatetime()` & `ISOdate()` to create date-times from numeric representations.
* Added `decimal_date()` and `date_decimal()`
* Added `make_difftime()` (duration constructor)
* Added duration helper functions: `dyears()`, `dmonths()`, `dweeks()`, `ddays()`, `dhours()`, `dminutes()`, `dseconds()`, `dmilliseconds()`, `dmicroseconds()`, `dnanoseconds()`.
* date-time functionality:
* Added `difftime` and `as.difftime()`
* Added `as.Date()` to convert to date
Expand Down
63 changes: 38 additions & 25 deletions r/R/dplyr-funcs-datetime.R
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,44 @@ register_bindings_duration <- function() {
})
}

.helpers_function_map <- list(
"dminutes" = list(60, "s"),
"dhours" = list(3600, "s"),
"ddays" = list(86400, "s"),
"dweeks" = list(604800, "s"),
"dmonths" = list(2629800, "s"),
"dyears" = list(31557600, "s"),
"dseconds" = list(1, "s"),
"dmilliseconds" = list(1, "ms"),
"dmicroseconds" = list(1, "us"),
"dnanoseconds" = list(1, "ns")
)
make_duration <- function(x, unit) {
x <- build_expr("cast", x, options = cast_options(to_type = int64()))
x$cast(duration(unit))
}
register_bindings_duration_helpers <- function() {
duration_helpers_map_factory <- function(value, unit) {
force(value)
force(unit)
function(x = 1) make_duration(x * value, unit)
}

for (name in names(.helpers_function_map)) {
register_binding(
name,
duration_helpers_map_factory(
.helpers_function_map[[name]][[1]],
.helpers_function_map[[name]][[2]]
)
)
}
Comment on lines +373 to +387
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This is actually even shorter than I though it would be!


register_binding("dpicoseconds", function(x = 1) {
abort("Duration in picoseconds not supported in Arrow.")
})
}

register_bindings_difftime_constructors <- function() {
register_binding("make_difftime", function(num = NULL,
units = "secs",
Expand Down Expand Up @@ -383,31 +421,6 @@ register_bindings_difftime_constructors <- function() {
})
}

make_duration <- function(x, unit) {
x <- build_expr("cast", x, options = cast_options(to_type = int64()))
x$cast(duration(unit))
}
register_bindings_duration_helpers <- function() {
register_binding("dminutes", function(x = 1) {
make_duration(x * 60, "s")
})
register_binding("dhours", function(x = 1) {
make_duration(x * 3600, "s")
})
register_binding("ddays", function(x = 1) {
make_duration(x * 86400, "s")
})
register_binding("dweeks", function(x = 1) {
make_duration(x * 604800, "s")
})
register_binding("dmonths", function(x = 1) {
make_duration(x * 2629800, "s")
})
register_binding("dyears", function(x = 1) {
make_duration(x * 31557600, "s")
})
}

binding_format_datetime <- function(x, format = "", tz = "", usetz = FALSE) {
if (usetz) {
format <- paste(format, "%Z")
Expand Down
80 changes: 79 additions & 1 deletion r/tests/testthat/test-dplyr-funcs-datetime.R
Original file line number Diff line number Diff line change
Expand Up @@ -1258,7 +1258,12 @@ test_that("`decimal_date()` and `date_decimal()`", {

test_that("dminutes, dhours, ddays, dweeks, dmonths, dyears", {
example_d <- tibble(x = c(1:10, NA))
date_to_add <- ymd("2009-08-03", tz = "America/Chicago")
date_to_add <- ymd("2009-08-03", tz = "Pacific/Marquesas")

# When comparing results we use ignore_attr = TRUE because of the diff in:
# attribute 'package' (absent vs. 'lubridate')
# class (difftime vs Duration)
# attribute 'units' (character vector ('secs') vs. absent)

compare_dplyr_binding(
.input %>%
Expand Down Expand Up @@ -1303,6 +1308,79 @@ test_that("dminutes, dhours, ddays, dweeks, dmonths, dyears", {
tibble(),
ignore_attr = TRUE
)

# double -> duration not supported in Arrow.
# Error is generated in the C++ code
expect_error(
test_df %>%
arrow_table() %>%
mutate(r_obj_dminutes = dminutes(1.12345)) %>%
collect()
)
Comment on lines +1312 to +1319
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this comment about why we are expect_error() but not actually asserting it (since this is all C++). 💯

})

test_that("dseconds, dmilliseconds, dmicroseconds, dnanoseconds, dpicoseconds", {
example_d <- tibble(x = c(1:10, NA))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also test what happens when we pass floats here too?

> lubridate::dseconds(1.5)
[1] "1.5s"

Seems to work, so we should ensure we can do that (or error helpfully if we can't for some reason)

Copy link
Member Author

@AlenkaF AlenkaF Apr 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, thanks for this!
Will search for discussions Dragos already had about casting float -> duration, then test and see =)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As duration type in Arrow is int64 and we can't pass floats here I will go with erroring helpfully. Will add it in the next commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARROW-16253 might be relevant here too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test for the error when the argument multiplied with the value of the multiplication factor of the duration helper function is float (went with easier solution - didn't go forward with forcing evaluation to check for type of an argument or try catching C++ error).

date_to_add <- ymd("2009-08-03", tz = "Pacific/Marquesas")

# When comparing results we use ignore_attr = TRUE because of the diff in:
# attribute 'package' (absent vs. 'lubridate')
# class (difftime vs Duration)
# attribute 'units' (character vector ('secs') vs. absent)

compare_dplyr_binding(
.input %>%
mutate(
dseconds = dseconds(x),
dmilliseconds = dmilliseconds(x),
dmicroseconds = dmicroseconds(x),
dnanoseconds = dnanoseconds(x),
) %>%
collect(),
example_d,
ignore_attr = TRUE
)

compare_dplyr_binding(
.input %>%
mutate(
dseconds = dseconds(x),
dmicroseconds = dmicroseconds(x),
new_date_1 = date_to_add + dseconds,
new_date_2 = date_to_add + dseconds - dmicroseconds,
new_duration = dseconds - dmicroseconds
) %>%
collect(),
example_d,
ignore_attr = TRUE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see this in the PR (though might have missed something), what attr are we ignoring? Maybe we should add a comment about what we're using that for

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a comment, you can wait with merging. But I have to remember, if I am honest =) Will do it tomorrow morning and add the comment then.

Thank you for the review!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using ignore_attr = TRUE due to the diff in attributes package, units and class: (difftime vs Duration). I added a comment about it in the beginning of both tests.

)

compare_dplyr_binding(
.input %>%
mutate(
r_obj_dseconds = dseconds(1),
r_obj_dmilliseconds = dmilliseconds(2),
r_obj_dmicroseconds = dmicroseconds(3),
r_obj_dnanoseconds = dnanoseconds(4)
) %>%
collect(),
tibble(),
ignore_attr = TRUE
)

expect_error(
call_binding("dpicoseconds"),
"Duration in picoseconds not supported in Arrow"
)

# double -> duration not supported in Arrow.
# Error is generated in the C++ code
expect_error(
test_df %>%
arrow_table() %>%
mutate(r_obj_dseconds = dseconds(1.12345)) %>%
collect()
)
})

test_that("make_difftime()", {
Expand Down