ARROW-14821: [R] Implement bindings for lubridate's floor_date, ceiling_date, and round_date

[djnavarro]

This patch provides dplyr bindings to for lubridate functions floor_date(), ceiling_date(), and round_date(). This is my first attempt at writing a patch, so my apologies if I've made any errors 🙂

Supported functionality:

• Allows rounding to integer multiples of common time units (second, minutes, days, etc)
• Mirrors the lubridate syntax allowing fractional units such as unit = .001 seconds as an alias for unit = 1 millisecond
• Allows partial matching of date units based on first three characters: e.g. sec, second, seconds all match second
• Mirrors lubridate in throwing errors when unit exceeds thresholds: 60 seconds, 60 minutes, 24 hours

### Major problems not yet addressed:

- Does not yet support the week_start argument, and implicitly fixes week_start = 4
- Does not yet mirror lubridate handling of timezones

I'd prefer to fix these two issues before merging, but I'm uncertain how best to handle them. Any advice would be appreciated!

### Minor things not yet addressed

- During rounding lubridate sometimes coerces Date objects to POSIXct. This is not mirrored in the arrow bindings: date32 classes remain date32 classes. This introduces minor differences in rounding in some cases
- Does not yet support the change_on_boundary argument to ceiling_date(). It's a small discrepancy, but it means that the default behaviour of the arrow dplyr binding mirrors lubridate prior to v1.6.0

EDIT: issues now addressed!

[github-actions]

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

[github-actions]

⚠️ Ticket has no components in JIRA, make sure you assign one.

[rok]

This looks great @djnavarro thanks for doing it!

Regarding your TODOs:

support week_start (essential)

Perhaps you can put in an R shim to get lubridate matching logic and open a Jira to fix this in c++ later on.

support timezones correctly (essential)

I found dates around 1900s quite problematic when testing against Pandas as they don't apply some historic offset changes. I'd suggest aiming at more modern times first.

support change_on_boundary argument to ceiling_date

This would probably be a good c++ Jira candidate too. Provide some examples if you have them handy :).

[rok]

Another thing regarding timezones - only timestamps in arrow have timezones. Dates do not.

[paleolimbot]

Just a few notes! I'm new to creating bindings, too, so take everything with a grain of salt. I noted a few style guide things...I think the linter check will give you notes on this when it runs as well.

[paleolimbot]

I think if statements elsewhere in arrow R code are always followed by a space (if (...) { ... })

[paleolimbot]

I think you can test this more directly with:

 expect_error(
call_binding("round_date", Expression$scalar(Sys.time()), "61 seconds"),
"Rounding with second > 60 is not supported"
)

[djnavarro]

Nice! I hadn't realised call_binding() existed 🤦🏻‍♀️ 😁

[paleolimbot]

I think this would be easier to find at the top of the file next to test_df

[paleolimbot]

The right way to go, I think!

[jonkeane]

Yup, I agree here. It might be nice to add a small comment in those test cases about this (so future-us remembers why we have as.Date() at the end there

[paleolimbot]

Suggested change

	}
	})

[paleolimbot]

@jonkeane Can you approve the workflows to run?

[ianmcook]

@jonkeane Can you approve the workflows to run?

I approved them

[thisisnic]

Good catch on these! What do you think about rephrasing a little to say what the user should do, and use the word "must" in there somewhere, kinda like this: https://style.tidyverse.org/error-messages.html ?

(note: feel free to say "no"; we don't do this in every single case)

[djnavarro]

Good thought. That said, I'm tempted to go with the suggestion @rok makes and allow these cases. It's not inconceivable that people might have a good reason to round to 90 minutes. I can't think of a reason why existing code would depend on this lubridate error?

[thisisnic]

I can see where you are coming from here. I guess, generally, the aim when implementing the functions for non-standard evaluation code blocks is to (as closely as possible unless there's good reason not to!) replicate the original. However, there are some rare cases when I've seen us not do that; for example, our mapping to lubridate::second() supports nanoseconds as we didn't see the harm in having more precise results, and so the unit test for that binding looks like this:

arrow/r/tests/testthat/test-dplyr-funcs-datetime.R

Lines 474 to 483 in 0c1fd88

	test_that("extract second from timestamp", {
	compare_dplyr_binding(
	.input %>%
	mutate(x= second(datetime)) %>%
	collect(),
	test_df,
	# arrow supports nanosecond resolution but lubridate does not
	tolerance=1e-6
	)
	})

That said, I also can't think of a reason someone else's code would depend on raising an error but I'm not confident that that would mean it won't - people have all sorts of unusual workflows.

There are multiple ways of calling Arrow C++ compute functions from R via the dplyr/non-standard evaluation code - either using the bindings to the lubridate etc equivalent, or calling the Arrow C++ compute function directly by prefixing the function name with arrow_ (so in this case it'd be arrow_round_temporal; if you've not come across this before, check it out here: https://arrow.apache.org/cookbook/r/manipulating-data---tables.html#use-arrow-functions-in-dplyr-verbs-in-arrow).

So if people use that directly they'd still have access to the full functionality. I'm not sure what to suggest here - would be good to get input from @jonkeane or @nealrichardson

[djnavarro]

That's a really good point. Maybe erring on the side of caution and mirroring lubridate as closely as possible would be best? It's going to come up again as an issue once I try to sort out the issue associated with the change_on_boundary argument, actually

[thisisnic]

Personally I'd err on the side of caution but I can see the argument for it not mattering - let's see what the more experienced R devs say!

Just to check; what's the issue with change_on_boundary? If it's the fact that it's not supported in the C++ yet, it's fine to open a C++ ticket for that to be implemented, raise an error in the R code saying that parameter is not yet supported, and open an R ticket for implementing it later once the C++ functionality is in place.

[rok]

Asking over at lubridate

[djnavarro]

@thisisnic: So yeah... I haven't finished my dive into the lubridate code on the change_on_boundary issue, but (I think!) the TL;DR is that it's an edge case for how ceiling_date() handles time points that sit at the start of a day. Should the ceiling of 2022-01-17 00:00:00 be 2022-01-18 00:00 or 2022-01-17 00:00? Conceptually, "January 17" is an interval of time that starts at 00:00 and ends at 23:59, so a strict interpretation suggests that all time points within that interval should ceiling to midnight of January 18. That's what lubridate currently does, but...

Prior to lubridate 1.6.0 ceiling_date() took a different approach, and AFAICT the C++ round_temporal() function does the same thing. If we were to ignore the strict interpretation of ISO 8601, we can think of "midnight moments" as analogous to integer values spaced along the real line, and round accordingly. Under this approach, which I think is more intuitive but does violate ISO 8601, we don't ceiling an integer up to the next integer, and so 2022-01-17 00:00 stays 2022-01-17 00:00 because it's already an "integer valued date".

The downside to violating ISO 8601 -- in addition to the obvious problem that it violates ISO 8601 -- is that it means we're no longer in agreement with lubridate. The downside to adhering to it is that you get counterintuitive result that taking a ceiling of a Date object always returns the next day:

ceiling_date(ymd("2022-01-17"), "day") # returns 2022-01-18

Long story short, the change_on_boundary argument exists so that lubridate can revert to its older behaviour. Given that the C++ round_temporal() only matches the older lubridate behaviour (again, assuming I've understood the code properly), I'll need to write a shim that supports the change_on_boundary anyway. If I don't we're not mirroring lubridate.

I think the right approach is probably the same one @rok suggested for week_start. Short term the solution is to write a shim to support it on the R side, and open a JIRA ticket for the C++ library

[thisisnic]

Sounds good!

[jonkeane]

What y'all have already said is great, and totally agree with this being an edge case for if extending just a bit is fine or if we should error in the same way.

How easy/hard would it be to construct the arrow_round_temporal() call if someone did want to do that?

[rok]

How about we allow such cases (minute > 60)? C++ implementation supports them and I can imagine I'd want to round to e.g. 90 minutes or 36 hours sometimes.
Users would probably not mind as we would not reduce capabilities.

[github-actions]

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

[github-actions]

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

[djnavarro]

I think I've taken this as far as I can right now? I had been hoping to write shims to support the week_start and change_on_boundary arguments but I'm not sure if that's possible right now. My original thought was to try something like this:

week_start_shim<-function(function_name, x, week_start, opts) {
offset<-Expression$create(Scalar$create(as.difftime(week_start-4, units="days")))
x<-Expression$create("add", x, offset)
x<-Expression$create(function_name, x, options=opts)
return(Expression$create("subtract", x, offset))
} 

I can't get anything like this to work because (as far as I can tell) we don't currently have the ability to add or subtract durations from date/time data in Arrow. I imagine I'm missing something obvious here!

[rok]

I think I've taken this as far as I can right now? I had been hoping to write shims to support the week_start and change_on_boundary arguments but I'm not sure if that's possible right now. My original thought was to try something like this:

week_start_shim<-function(function_name, x, week_start, opts) {
offset<-Expression$create(Scalar$create(as.difftime(week_start-4, units="days")))
x<-Expression$create("add", x, offset)
x<-Expression$create(function_name, x, options=opts)
return(Expression$create("subtract", x, offset))
} 

I can't get anything like this to work because (as far as I can tell) we don't currently have the ability to add or subtract durations from date/time data in Arrow. I imagine I'm missing something obvious here!

Would it make sense for the shims to be in R?
For add/subtract in arrow see ARROW-11090.

[thisisnic]

I think I've taken this as far as I can right now? I had been hoping to write shims to support the week_start and change_on_boundary arguments but I'm not sure if that's possible right now. My original thought was to try something like this:

week_start_shim<-function(function_name, x, week_start, opts) {
offset<-Expression$create(Scalar$create(as.difftime(week_start-4, units="days")))
x<-Expression$create("add", x, offset)
x<-Expression$create(function_name, x, options=opts)
return(Expression$create("subtract", x, offset))
} 

I can't get anything like this to work because (as far as I can tell) we don't currently have the ability to add or subtract durations from date/time data in Arrow. I imagine I'm missing something obvious here!

If the functionality isn't there, I'd be tempted to put this PR aside for the moment - we're aiming to have a larger number of the lubridate and stringr functions implemented for the 8.0.0 release (i.e. ~mid-April) anyway, so there's no rush with this and there's no need to spend ages working around the missing functionality (unless you particularly want to!) when it's totally fine to wait for it to be implemented in C++ anyway. Sorry though, I can imagine that's a little frustrating on the first ticket you've picked up!

[djnavarro]

My thinking at this point is that (if this is consistent with the general etiquette here!) I'd like to (1) work out why the build is failing two checks and fix those, (2) merge the passing version, (3) open tickets for the missing functionality, one for the C++ side and one for the R side (and assign myself to the R one 😁 ).

My reasons for suggesting this are:

• The current version does work for most use cases. The week_start issue only applies when the rounding unit is "week". As it stands we can round to "7 days" or indeed any time unit except "week" and have it mirror lubridate almost exactly. I've modified the PR so that we abandon ship if the user specifies "week" as the rounding unit, and falls back on lubridate.
• It gives me an excuse to do a deeper dive on timezones before merging the current version: if I'm going to write more lubridate bindings later this is something I'm going to need to know anyway!
• Once it's merged it would be clearer what modifications are needed to supply the missing functionality, so I can write the JIRA tickets a bit more precisely
• A little selfishly... I presume that once I am no longer a first-time contributor I won't need to constantly bug maintainers to enable the GH actions workflows
• Slightly less selfishly, it would be mildly convenient for writing blog content about the process. I think there's value in highlighting that a PR can still be useful when surfaces issues pertaining to missing functionality and in doing so immediately triggers new issues?

Possible reasons not to:

• As flagged above I'm not sure of the etiquette here. If it's not considered appropriate I'm totally happy to leave this hanging!
• The change_on_boundary issue does mean that the Arrow binding for ceiling_date() introduces a minor discrepancy. We're currently mirroring a slightly older lubridate default. I'm not sure that's currently a big issue, but it's something we'd definitely want to revisit before this appears in a release

I'm happy to follow advice here. I'm totally fine with whatever approach more experienced folks think is best! 🙂

[thisisnic]

In case it helps, you could try searching for "ARROW-13168" in the test files as there are some tests we skip on Windows because of the issue mentioned in that ticket - not sure if this is related or not though.

[rok]

@djnavarro I've opened a jira for the week_start issue.

[rok]

@djnavarroweek_starts_monday for RoundTemporalOptions was just merged: https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/api_scalar.h#L107-L120

Does this unblock this PR or do we need to solve change_on_boundary first?

Let me know if I can help somehow.

[djnavarro]

@rok sorry for not getting to this earlier!

For the week_start issue: if I'm reading it correctly, we may need a little more flexibility on the C++ side to allow the user to specify an integer between 1 (Monday) and 7 (Sunday). For example, this is the lubridate::floor_date() behaviour:

this_thursday<- ymd("2022-03-10")
floor_date(this_thursday, unit="week", week_start=1) # returns "2022-03-07"
floor_date(this_thursday, unit="week", week_start=2) # returns "2022-03-08"

It's still possible I can handle it at the level of the R binding (I've learned a bit more in the last couple of month!) but I'm wondering if the R level is the best place to do this or if it's better to implement it at the C++ level?

For the change_on_boundary issue: my intuition is that this is best implemented as part of the R binding because the "behaviour of ceiling at the boundary" feels so specific to lubridate? As far as I can tell though it's still blocked because there's no way to add +1 time unit to a temporal object, right? (Or am I missing something???)

[rok]

No worries @djnavarro :)
About week_start - are you sure we need anything but 1 and 7 (which maps to week_starts_monday=true/false respectively)? It's relatively straight forward to implement in c++ (and we should probably do it there) but does anyone ever need that? I'm honestly curious. Maybe someone does need it?

About the change_on_boundary - would that be useful for rollup too? See my comment here. Add/subtract kernels can now handle temporal types so you could use them to subtract a single time unit of duration from a timestamp if that's what you'd need.

[djnavarro]

Well, I can't think of a good reason why anyone would set week_start to any value other than 1 or 7, but technically lubridate allows it and I suppose there's the possibility that there exists some code that has the week starting on a Wednesday. That being said, if we can now add and subtract durations to temporal types, I should be able to call add_checked() to handle the edge cases, right?

[rok]

I'd raise on (week_start != 1 || week_start !=7) just to see who opens the ticket and why do they need this :). On a serious note we can change the c++ logic if we here decide to.

I'm not sure what you mean by edge cases. Could you sketch it with some pseudocode?

[djnavarro]

Maybe it's easiest to illustrate with actual R code. The behaviour I need to emulate with regards to the change_on_boundary argument looks different depending on whether the data are stored as a datetime (e.g. POSIXct in R, timestamp in Arrow) versus a Date (e.g., Date in R, date32 in Arrow):

# a datetime object at the midnight boundary, and a date object with no timemidnight_time<- as.POSIXct(strptime("2022-03-10 00:00:00", format="%F %T"))
timeless_date<- as.Date("2022-03-10")
# ceiling date applied to the datetime
ceiling_date(midnight_time, unit="day", change_on_boundary=NULL) # returns the 10th
ceiling_date(midnight_time, unit="day", change_on_boundary=TRUE) # returns the 11th
ceiling_date(midnight_time, unit="day", change_on_boundary=FALSE) # returns the 10th# ceiling date applied to the date
ceiling_date(timeless_date, unit="day", change_on_boundary=NULL) # returns the 11th
ceiling_date(timeless_date, unit="day", change_on_boundary=TRUE) # returns the 11th
ceiling_date(timeless_date, unit="day", change_on_boundary=FALSE) # returns the 10th

This pattern only affects times and dates deemed to be on the boundary with respect to the rounding unit. Any time past midnight and lubridate::ceiling_date() would always return the 11th for the datetime object.

Currently, the ceil_temporal C++ function would return the 10th for the analogous cases. If I can work out how to add +1 time unit to the boundary cases then I have no problems exactly mimicking lubridate behaviour. I can get this to work for timestamp objects. This code works, for instance, and adds 1 day to the Arrow timestamp that corresponds to midnight_time:

call_function("add_checked", Scalar$create(midnight_time), Scalar$create(as.difftime("24:00:00")))

But when I try the same thing for the date, however, it throws this error:

Error: NotImplemented: Function 'add_checked' has no kernel matching input types (scalar[date32[day]], scalar[duration[s]])

That seems odd to me and I'm not sure why I'm getting that error because I'm pretty sure you implemented this in ARROW-14901. So I must be doing something wrong?

EDIT: Never mind, I worked it out. 🤦🏻‍♀️ I can handle the change_on_boundary behaviour on the R side!

[rok]

That's great news @djnavarro!

Does solving this with subtraction work correctly around DST changes? e.g. (stealing an example from here):

2018-03-25 02:10:00 is in a gap between
2018-03-25 02:00:00 CET and
2018-03-25 03:00:00 CEST which are both equivalent to
2018-03-25 01:00:00 UTC

We should also consider performance implications here. Adding an option to the c++ kernels might be better than calling two different c++ kernels. However that should be a separate ticket.

[djnavarro]

@rok Agreed! My thinking at the moment is to see how far I can push this from the R side so that we have a clear idea of exactly what behaviour we're trying to emulate, but we'll certainly want to think about how to improve performance because I can guarantee that the code I'm writing on the R side isn't very efficient.

Maybe the right thing to do is to arrange a voice chat sometime soon?

[rok]

@rok Agreed! My thinking at the moment is to see how far I can push this from the R side so that we have a clear idea of exactly what behaviour we're trying to emulate, but we'll certainly want to think about how to improve performance because I can guarantee that the code I'm writing on the R side isn't very efficient.

Perfect. Understanding the behaviour is enough at this stage. And current situation PR is pretty efficient IMO.

[rok]

@djnavarro I've rebased to fix the C++ conflicts and removed the ARROW-16412 workarounds.
Let me know if another bug pops up :).

[djnavarro]

Okay, I think this is actually good? The failing check doesn't appear to be related to this PR as far as I can tell, but notably one of them is timezone related so there might be something going on that I am not quite understanding

2022-07-21T00:50:55.3146284Z ══ Warnings ════════════════════════════════════════════════════════════════════
2022-07-21T00:50:55.3146568Z ── Warning (test-Array.R:269:3): array uses local timezone for POSIXct without timezone ──
2022-07-21T00:50:55.3146749Z running command 'timedatectl' had status 1
2022-07-21T00:50:55.3146829Z Backtrace:
2022-07-21T00:50:55.3146936Z 1. withr::with_envvar(...)
2022-07-21T00:50:55.3147080Z at test-Array.R:269:2
2022-07-21T00:50:55.3147170Z 6. base (local) `<fn>`()
2022-07-21T00:50:55.3147297Z 7. base::system("timedatectl", intern = TRUE)
2022-07-21T00:50:55.3147562Z ── Warning (test-RecordBatch.R:644:3): Handling string data with embedded nuls ──
2022-07-21T00:50:55.3147887Z Stripping '\0' (nul) from character vector
2022-07-21T00:50:55.3147962Z Backtrace:
2022-07-21T00:50:55.3148060Z 1. withr::with_options(...)
2022-07-21T00:50:55.3148213Z at test-RecordBatch.R:644:2
2022-07-21T00:50:55.3148314Z 9. arrow:::expect_equal(...)
2022-07-21T00:50:55.3148416Z 10. testthat::expect_equal(...)
2022-07-21T00:50:55.3148679Z at tests/testthat/helper-expectation.R:42:4
2022-07-21T00:50:55.3148864Z 11. testthat:::expect_waldo_equal("equal", act, exp, info, ..., tolerance = tolerance)
2022-07-21T00:50:55.3148968Z 12. testthat:::waldo_compare(...)
2022-07-21T00:50:55.3149106Z 13. waldo::compare(x, y, ..., x_arg = x_arg, y_arg = y_arg)
2022-07-21T00:50:55.3149267Z 14. waldo:::compare_structure(x, y, paths = c(x_arg, y_arg), opts = opts)
2022-07-21T00:50:55.3149373Z 15. waldo:::is_identical(x, y, opts)
2022-07-21T00:50:55.3149380Z 

[rok]

This has been observed in otherPRs so I think it's not related.

LGTM!

[nealrichardson]

LGTM, thanks for persisting!

[rok]

Thanks for doing this @djnavarro ! 😄

[djnavarro]

Woohoo! 🎉

[ursabot]

Benchmark runs are scheduled for baseline = 1f03f12 and contender = b0734e6. b0734e6 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.17% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.46% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Failed] b0734e68 ec2-t3-xlarge-us-east-2
[Failed] b0734e68 test-mac-arm
[Failed] b0734e68 ursa-i9-9960x
[Finished] b0734e68 ursa-thinkcentre-m75q
[Failed] 1f03f126 ec2-t3-xlarge-us-east-2
[Failed] 1f03f126 test-mac-arm
[Failed] 1f03f126 ursa-i9-9960x
[Finished] 1f03f126 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java