jonkeane commented on a change in pull request #12431:
URL: https://github.com/apache/arrow/pull/12431#discussion_r811997145
##########
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:
While I totally agree `"kernel"` is likely to be unfamiliar — there is
other content in this message that IMO conveys what's going on. Do we think
that `"kernel"` will confuse people enough that they wouldn't understand the
rest of the message or stump them such that they don't look at the rest of the
message? I guess my approach for something like this would be trying to read
the rest of the message and it's (not a grammatical) sentence, but does
describe it in basic ways (albeit our type info has some extra stuff about
array/scalar).
I'm legit on the fence myself about this, I don't know the right answer
here! If we do want to smooth these out and remove the word `kernel` I would
suggest we do that as a separate Jira so we can (1) get a few more eyes on the
decision and people are aware of it going forward. (2) we likely have _many_
circumstances where messages like these show up in the package (and many of
those aren't tested since we don't test things directly from C++). So we will
want to do something generally to approach this (we don't necessarily want to
add type-checking in front of every C++ call in the package and writing out a
new message each time we do that)
--
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
