jonkeane commented on a change in pull request #12431:
URL: https://github.com/apache/arrow/pull/12431#discussion_r810508212
##########
File path: r/tests/testthat/test-dplyr-funcs-datetime.R
##########
@@ -711,3 +711,40 @@ test_that("am/pm mirror lubridate", {
)
})
+test_that("dst extracts daylight savings time correctly", {
+ test_df <- tibble(
+ dates = as.POSIXct(c("2021-02-20", "2021-07-31", "2021-10-31",
"2021-01-31"), tz = "Europe/London")
+ )
+ # https://issues.apache.org/jira/browse/ARROW-13168
+ skip_on_os("windows")
+
+ compare_dplyr_binding(
+ .input %>%
+ mutate(dst = dst(dates)) %>%
+ collect(),
+ test_df
+ )
+})
+
+test_that("dst errors with unsupported input", {
+ expect_error(
+ call_function("is_dst", Scalar$create("this is a string, not a
timestamp")),
+ "NotImplemented: Function 'is_dst' has no kernel matching input types
(scalar[string])",
+ fixed = TRUE
+ )
+ expect_error(
+ call_function("is_dst", Scalar$create(1L)),
+ "NotImplemented: Function 'is_dst' has no kernel matching input types
(scalar[int32])",
+ fixed = TRUE
+ )
+ expect_error(
+ call_function("is_dst", Scalar$create(2.2)),
+ "NotImplemented: Function 'is_dst' has no kernel matching input types
(scalar[double])",
+ fixed = TRUE
+ )
+ expect_error(
+ call_function("is_dst", Scalar$create(TRUE)),
+ "NotImplemented: Function 'is_dst' has no kernel matching input types
(scalar[bool])",
+ fixed = TRUE
+ )
Review comment:
Awesome, thanks for these! How do you feel about those error messages?
Do you think they are clear enough to our end users about what's going on with
those inputs?
I think they are pretty clear (though they aren't the absolute best), but at
this point we weigh: do we add extra type checking to the binding _solely_ to
improve these error messages or not? (Or put another way are these error
messages so unintelligible that we should take on the additional code burden of
additional type checking and error production in R?). What do you think?
If we don't add the extra type checking and error production in R then we
don't need to add tests here (since they are effectively duplicating the C++
error handling tests which presumably exist around these already). (Though
thank you for plopping them in the PR — it makes it much easier to talk about
here even if we plopped them in only to remove them later!)
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
