You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/11/02 15:27:08 UTC
[GitHub] [arrow] paleolimbot opened a new pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
paleolimbot opened a new pull request #11592:
URL: https://github.com/apache/arrow/pull/11592
Implements `is.Date()`, `is.POSIXct()`, `is.timepoint()`, and `is.instant()` from the lubridate package.
There is also `lubridate::is.POSIXt()` and `lubridate::is.POSIXlt()`...these are a bit more niche and I thought they might be confusing if implemented here.
Another question I had while implementing this...can/should these return an `Expression$scalar()`? This isn't how the other `is.*()` functions are done but maybe this makes a difference?
``` r
library(arrow, warn.conflicts = FALSE)
library(lubridate, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)
test_date <- as.POSIXct("2017-01-01 00:00:11.3456789", tz = "Pacific/Marquesas")
test_df <- tibble::tibble(
datetime = c(test_date, NA) + 1,
date = c(as.Date("2021-09-09"), NA)
)
RecordBatch$create(test_df) %>%
mutate(is.POSIXct(datetime), is.POSIXct(date)) %>%
collect()
#> # A tibble: 2 × 4
#> datetime date `is.POSIXct(datetime)` `is.POSIXct(date)`
#> <dttm> <date> <lgl> <lgl>
#> 1 2017-01-01 00:00:12 2021-09-09 TRUE FALSE
#> 2 NA NA TRUE FALSE
RecordBatch$create(test_df) %>%
mutate(is.Date(datetime), is.Date(date)) %>%
collect()
#> # A tibble: 2 × 4
#> datetime date `is.Date(datetime)` `is.Date(date)`
#> <dttm> <date> <lgl> <lgl>
#> 1 2017-01-01 00:00:12 2021-09-09 FALSE TRUE
#> 2 NA NA FALSE TRUE
RecordBatch$create(test_df) %>%
mutate(is.instant(datetime), is.instant(date)) %>%
collect()
#> # A tibble: 2 × 4
#> datetime date `is.instant(datetime)` `is.instant(date)`
#> <dttm> <date> <lgl> <lgl>
#> 1 2017-01-01 00:00:12 2021-09-09 TRUE TRUE
#> 2 NA NA TRUE TRUE
RecordBatch$create(test_df) %>%
mutate(is.timepoint(datetime), is.timepoint(date)) %>%
collect()
#> # A tibble: 2 × 4
#> datetime date `is.timepoint(datetime)` `is.timepoint(date)`
#> <dttm> <date> <lgl> <lgl>
#> 1 2017-01-01 00:00:12 2021-09-09 TRUE TRUE
#> 2 NA NA TRUE TRUE
```
<sup>Created on 2021-11-02 by the [reprex package](https://reprex.tidyverse.org) (v2.0.1)</sup>
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r741223425
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
It's already in `Suggests` which seems to be the right place for this? (i.e., no namespace load until the function is called and since this is a translation of a lubridate function, it's unlikely this will get called by a user without lubridate installed?)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-959720651
Are there any changes on my end required to move this forward? I don't know why R / AMD64 Windows R RTools 35 is failing and I don't seem to have the ability to re-run the job to see if it's intermittent.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r742145105
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
I removed all references to `lubridate::`...super easy to avoid in this case!
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
I removed all references to `lubridate::`...super easy to avoid in this case!
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-957836044
> Another question I had while implementing this...can/should these return an `Expression$scalar()`? This isn't how the other `is.*()` functions are done but maybe this makes a difference?
The `is.*()` functions are different for reasons that are described (but maybe not very clearly) at ARROW-13186.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-957836044
> Another question I had while implementing this...can/should these return an `Expression$scalar()`? This isn't how the other `is.*()` functions are done but maybe this makes a difference?
The `is.*()` functions are different for reasons that are described (but maybe not very clearly) at ARROW-13186.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r743028399
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
+}
+
+nse_funcs$is.instant <- nse_funcs$is.timepoint <- function(x) {
+ inherits(x, c("POSIXt", "Date")) ||
Review comment:
Good catch!
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-957997188
Ok! I think this is cleaner...I had incorrectly included TIME32 and TIME64 with TIMESTAMP as well. Does DATE64 map back to POSIXct?
I think it's best to leave POSIXlt alone, since it maps to a `struct<...>` and didn't work with `strftime()` anyway:
``` r
library(arrow, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)
RecordBatch$create(x = as.POSIXct(as.Date("2000-01-01"))) %>%
mutate(y = strftime(x, "%Y-%M-%d")) %>%
collect()
#> # A tibble: 1 × 2
#> x y
#> <dttm> <chr>
#> 1 2000-01-01 00:00:00 2000-00-01
RecordBatch$create(x = as.POSIXlt(as.Date("2000-01-01"))) %>%
mutate(y = strftime(x, "%Y-%M-%d")) %>%
collect()
#> Error: NotImplemented: Function strftime has no kernel matching input types (array[struct<sec: double, min: int32, hour: int32, mday: int32, mon: int32, year: int32, wday: int32, yday: int32, isdst: int32>])
#> /Users/deweydunnington/Desktop/rscratch/arrow/cpp/src/arrow/compute/exec/expression.cc:340 call.function->DispatchBest(&descrs)
#> /Users/deweydunnington/Desktop/rscratch/arrow/cpp/src/arrow/compute/exec/project_node.cc:67 expr.Bind(*inputs[0]->output_schema())
```
<sup>Created on 2021-11-02 by the [reprex package](https://reprex.tidyverse.org) (v2.0.1)</sup>
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r741223425
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
It's already in `Suggests` which seems to be the right place for this? (i.e., no namespace load until the function is called and since this is a translation of a lubridate function, it's unlikely this will get called by a user without lubridate installed?)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r741269486
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
We don't have this in our CI yet, but would be a test case for https://issues.apache.org/jira/browse/ARROW-14544
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r743031219
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
+}
+
+nse_funcs$is.instant <- nse_funcs$is.timepoint <- function(x) {
+ inherits(x, c("POSIXt", "POSIXct", "POSIXlt", "Date")) ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP", "DATE32")])
+}
+
+nse_funcs$is.POSIXct <- function(x) {
+ inherits(x, "POSIXct") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP")])
+}
+
Review comment:
Likewise with `is.POSIXlt()` which is defined in lubridate as `inherits(x, "POSIXlt")`
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-962179421
Benchmark runs are scheduled for baseline = 230afef57f0ccc2135ced23093bac4298d5ba9e4 and contender = e907a9f0786cbc5869ed35b0a6cbfa946d89a419. e907a9f0786cbc5869ed35b0a6cbfa946d89a419 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/bbe0648555f74c47b4b6d3076d897bc3...e13ed74d618c4aa9ba1e96898b570687/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/d782fdc52e314ffdb21748ee29313c18...93cdb86ac8c940aea514f7de73702b8c/)
[Finished :arrow_down:0.31% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/cf78f551db64443799c96410f6bb6ac9...0560939c8838406d9394517bdda5fd38/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r742145105
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
I removed all references to `lubridate::`...super easy to avoid in this case!
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
jonkeane commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-957908069
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r741201435
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
lubridate isn't in the arrow imports or depends so I don't think we can call lubridate functions it like this. I think we need to use `class()` or `inherits()` to determine if `x` is a lubridate object when `x` is an R object not an arrow object.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r743032655
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
+}
+
+nse_funcs$is.instant <- nse_funcs$is.timepoint <- function(x) {
+ inherits(x, c("POSIXt", "POSIXct", "POSIXlt", "Date")) ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP", "DATE32")])
+}
+
+nse_funcs$is.POSIXct <- function(x) {
+ inherits(x, "POSIXct") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP")])
+}
+
Review comment:
Oh, I didn't see your other comments about this. Disregard this.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r741265735
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
Unlikely, but it shouldn't fail if you don't have lubridate installed
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-957997188
Ok! I think this is cleaner...I had incorrectly included TIME32 and TIME64 with TIMESTAMP as well. Does DATE64 map back to POSIXct?
I think it's best to leave POSIXlt alone, since it maps to a `struct<...>` and didn't work with `strftime()` anyway:
``` r
library(arrow, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)
RecordBatch$create(x = as.POSIXct(as.Date("2000-01-01"))) %>%
mutate(y = strftime(x, "%Y-%M-%d")) %>%
collect()
#> # A tibble: 1 × 2
#> x y
#> <dttm> <chr>
#> 1 2000-01-01 00:00:00 2000-00-01
RecordBatch$create(x = as.POSIXlt(as.Date("2000-01-01"))) %>%
mutate(y = strftime(x, "%Y-%M-%d")) %>%
collect()
#> Error: NotImplemented: Function strftime has no kernel matching input types (array[struct<sec: double, min: int32, hour: int32, mday: int32, mon: int32, year: int32, wday: int32, yday: int32, isdst: int32>])
#> /Users/deweydunnington/Desktop/rscratch/arrow/cpp/src/arrow/compute/exec/expression.cc:340 call.function->DispatchBest(&descrs)
#> /Users/deweydunnington/Desktop/rscratch/arrow/cpp/src/arrow/compute/exec/project_node.cc:67 expr.Bind(*inputs[0]->output_schema())
```
<sup>Created on 2021-11-02 by the [reprex package](https://reprex.tidyverse.org) (v2.0.1)</sup>
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r741269486
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
We don't have this in our CI yet, but would be a test case for https://issues.apache.org/jira/browse/ARROW-14544
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r743034726
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
Review comment:
I think we want `DATE64` in here as well since those are treated as dates in other areas (even though they do have a bit funny millisecond precision). Possibly also below for `is.instant()` too?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-957822026
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-957822026
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r742145105
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
I removed all references to `lubridate::`...super easy to avoid in this case!
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r743029015
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
+}
+
+nse_funcs$is.instant <- nse_funcs$is.timepoint <- function(x) {
+ inherits(x, c("POSIXt", "POSIXct", "POSIXlt", "Date")) ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP", "DATE32")])
+}
+
+nse_funcs$is.POSIXct <- function(x) {
+ inherits(x, "POSIXct") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP")])
+}
+
Review comment:
Should we add a binding for `is.POSIXt()`? It's defined in lubridate as `inherits(x, "POSIXt")`
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r743034726
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
Review comment:
I think we want `DATE64` in here as well since those are treated as dates in other areas (even though they do have a bit funny millisecond precision). Possibly also below for `is.instant()` too?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-957822026
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r741268531
##########
File path: r/tests/testthat/test-dplyr-funcs-datetime.R
##########
@@ -44,6 +44,38 @@ test_df <- tibble::tibble(
date = c(as.Date("2021-09-09"), NA)
)
+# These tests test detection of dates and times
+
+test_that("is.* functions from lubridate", {
+ compare_dplyr_binding(
+ .input %>%
+ mutate(x = is.POSIXct(datetime)) %>%
Review comment:
Should these also test the FALSE cases?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r741201435
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
lubridate isn't in the arrow imports or depends so I don't think we can call lubridate functions it like this. I think we need to use `class()` or `inherits()` to determine if `x` is a lubridate object when `x` is an R object not an arrow object.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r743028399
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
+}
+
+nse_funcs$is.instant <- nse_funcs$is.timepoint <- function(x) {
+ inherits(x, c("POSIXt", "Date")) ||
Review comment:
Good catch!
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
+}
+
+nse_funcs$is.instant <- nse_funcs$is.timepoint <- function(x) {
+ inherits(x, c("POSIXt", "POSIXct", "POSIXlt", "Date")) ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP", "DATE32")])
+}
+
+nse_funcs$is.POSIXct <- function(x) {
+ inherits(x, "POSIXct") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP")])
+}
+
Review comment:
@jonkeane and I chatted about that earlier in thie PR...I think it could be added but only if the POSIXlt class was converted to Arrow as a timestamp (currently it gets converted to a struct). Otherwise I think it's too hard to pay off here (do you detect a very specific struct type?).
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-962179421
Benchmark runs are scheduled for baseline = 230afef57f0ccc2135ced23093bac4298d5ba9e4 and contender = e907a9f0786cbc5869ed35b0a6cbfa946d89a419. e907a9f0786cbc5869ed35b0a6cbfa946d89a419 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/bbe0648555f74c47b4b6d3076d897bc3...e13ed74d618c4aa9ba1e96898b570687/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/d782fdc52e314ffdb21748ee29313c18...93cdb86ac8c940aea514f7de73702b8c/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/cf78f551db64443799c96410f6bb6ac9...0560939c8838406d9394517bdda5fd38/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-962179421
Benchmark runs are scheduled for baseline = 230afef57f0ccc2135ced23093bac4298d5ba9e4 and contender = e907a9f0786cbc5869ed35b0a6cbfa946d89a419. e907a9f0786cbc5869ed35b0a6cbfa946d89a419 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/bbe0648555f74c47b4b6d3076d897bc3...e13ed74d618c4aa9ba1e96898b570687/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/d782fdc52e314ffdb21748ee29313c18...93cdb86ac8c940aea514f7de73702b8c/)
[Finished :arrow_down:0.31% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/cf78f551db64443799c96410f6bb6ac9...0560939c8838406d9394517bdda5fd38/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-959720651
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r743027283
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
+}
+
+nse_funcs$is.instant <- nse_funcs$is.timepoint <- function(x) {
+ inherits(x, c("POSIXt", "Date")) ||
Review comment:
For exact parity with lubridate, this would be
```suggestion
inherits(x, c("POSIXt", "POSIXct", "POSIXlt", "Date")) ||
```
(https://github.com/tidyverse/lubridate/blob/9cece5680d0280c5cd1ccdbc9309ccb4724b3a34/R/instants.r#L15)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r741269486
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
We don't have this in our CI yet, but would be a test case for https://issues.apache.org/jira/browse/ARROW-14544
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r741201435
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
lubridate isn't in the arrow imports or depends so I don't think we can call lubridate functions it like this. I think we need to use `class()` or `inherits()` to determine if `x` is a lubridate object when `x` is an R object not an arrow object.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-959720651
Are there any changes on my end required to move this forward? I don't know why R / AMD64 Windows R RTools 35 is failing and I don't seem to have the ability to re-run the job to see if it's intermittent.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
jonkeane commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-957908069
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r743027283
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
+}
+
+nse_funcs$is.instant <- nse_funcs$is.timepoint <- function(x) {
+ inherits(x, c("POSIXt", "Date")) ||
Review comment:
For exact parity with lubridate, this would be
```suggestion
inherits(x, c("POSIXt", "POSIXct", "POSIXlt", "Date")) ||
```
(https://github.com/tidyverse/lubridate/blob/9cece5680d0280c5cd1ccdbc9309ccb4724b3a34/R/instants.r#L15)
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
+}
+
+nse_funcs$is.instant <- nse_funcs$is.timepoint <- function(x) {
+ inherits(x, c("POSIXt", "POSIXct", "POSIXlt", "Date")) ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP", "DATE32")])
+}
+
+nse_funcs$is.POSIXct <- function(x) {
+ inherits(x, "POSIXct") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP")])
+}
+
Review comment:
Should we add a binding for `is.POSIXt()`? It's defined in lubridate as `inherits(x, "POSIXt")`
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
+}
+
+nse_funcs$is.instant <- nse_funcs$is.timepoint <- function(x) {
+ inherits(x, c("POSIXt", "POSIXct", "POSIXlt", "Date")) ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP", "DATE32")])
+}
+
+nse_funcs$is.POSIXct <- function(x) {
+ inherits(x, "POSIXct") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP")])
+}
+
Review comment:
Likewise with `is.POSIXlt()` which is defined in lubridate as `inherits(x, "POSIXlt")`
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
+}
+
+nse_funcs$is.instant <- nse_funcs$is.timepoint <- function(x) {
+ inherits(x, c("POSIXt", "POSIXct", "POSIXlt", "Date")) ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP", "DATE32")])
+}
+
+nse_funcs$is.POSIXct <- function(x) {
+ inherits(x, "POSIXct") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP")])
+}
+
Review comment:
Oh, I didn't see your other comments about this. Disregard this.
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
+}
+
+nse_funcs$is.instant <- nse_funcs$is.timepoint <- function(x) {
+ inherits(x, c("POSIXt", "POSIXct", "POSIXlt", "Date")) ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP", "DATE32")])
+}
+
+nse_funcs$is.POSIXct <- function(x) {
+ inherits(x, "POSIXct") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP")])
+}
+
Review comment:
Oh, I didn't see your other comments about `is.POSIXlt()`. Disregard that. I'm still not sure about `is.POSIXt()` though.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r743033920
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
+}
+
+nse_funcs$is.instant <- nse_funcs$is.timepoint <- function(x) {
+ inherits(x, c("POSIXt", "POSIXct", "POSIXlt", "Date")) ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP", "DATE32")])
+}
+
+nse_funcs$is.POSIXct <- function(x) {
+ inherits(x, "POSIXct") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP")])
+}
+
Review comment:
@jonkeane and I chatted about that earlier in thie PR...I think it could be added but only if the POSIXlt class was converted to Arrow as a timestamp (currently it gets converted to a struct). Otherwise I think it's too hard to pay off here (do you detect a very specific struct type?).
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r743032655
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +844,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ inherits(x, "Date") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("DATE32")])
+}
+
+nse_funcs$is.instant <- nse_funcs$is.timepoint <- function(x) {
+ inherits(x, c("POSIXt", "POSIXct", "POSIXlt", "Date")) ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP", "DATE32")])
+}
+
+nse_funcs$is.POSIXct <- function(x) {
+ inherits(x, "POSIXct") ||
+ (inherits(x, "Expression") && x$type_id() %in% Type[c("TIMESTAMP")])
+}
+
Review comment:
Oh, I didn't see your other comments about `is.POSIXlt()`. Disregard that. I'm still not sure about `is.POSIXt()` though.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-962179421
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
jonkeane commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-957908069
This is looking good. On `lubridate::is.POSIXt()` being more niche, I think we might want them. I implemented [a (hacky) version](https://github.com/apache/arrow/blob/a0c650415bc28920512077faecdfa9d07d3c4efe/r/R/dplyr-functions.R#L258-L261) of that in order to [prevent dates from getting timestamped in `strftime`](https://github.com/apache/arrow/blob/a0c650415bc28920512077faecdfa9d07d3c4efe/r/R/dplyr-functions.R#L776-L782). What about them were you thinking would be confusing?
The name `is_timestamp` doesn't need to remain the same, I purposely didn't use `lubridate::is.POSIXt()` since we would come back, but if we can + want to use that instead here that would be totally fine!
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r741265735
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
Unlikely, but it shouldn't fail if you don't have lubridate installed
##########
File path: r/tests/testthat/test-dplyr-funcs-datetime.R
##########
@@ -44,6 +44,38 @@ test_df <- tibble::tibble(
date = c(as.Date("2021-09-09"), NA)
)
+# These tests test detection of dates and times
+
+test_that("is.* functions from lubridate", {
+ compare_dplyr_binding(
+ .input %>%
+ mutate(x = is.POSIXct(datetime)) %>%
Review comment:
Should these also test the FALSE cases?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r741265735
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
Unlikely, but it shouldn't fail if you don't have lubridate installed
##########
File path: r/tests/testthat/test-dplyr-funcs-datetime.R
##########
@@ -44,6 +44,38 @@ test_df <- tibble::tibble(
date = c(as.Date("2021-09-09"), NA)
)
+# These tests test detection of dates and times
+
+test_that("is.* functions from lubridate", {
+ compare_dplyr_binding(
+ .input %>%
+ mutate(x = is.POSIXct(datetime)) %>%
Review comment:
Should these also test the FALSE cases?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane closed pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
jonkeane closed pull request #11592:
URL: https://github.com/apache/arrow/pull/11592
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#discussion_r741223425
##########
File path: r/R/dplyr-functions.R
##########
@@ -848,6 +848,21 @@ nse_funcs$wday <- function(x,
Expression$create("day_of_week", x, options = list(count_from_zero = FALSE, week_start = week_start))
}
+nse_funcs$is.Date <- function(x) {
+ lubridate::is.Date(x) ||
Review comment:
It's already in `Suggests` which seems to be the right place for this? (i.e., no namespace load until the function is called and since this is a translation of a lubridate function, it's unlikely this will get called by a user without lubridate installed?)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-957836044
> Another question I had while implementing this...can/should these return an `Expression$scalar()`? This isn't how the other `is.*()` functions are done but maybe this makes a difference?
The `is.*()` functions are different for reasons that are described (but maybe not very clearly) at ARROW-13186.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on pull request #11592: ARROW-14227: [R] Implement lubridate is.* methods
Posted by GitBox <gi...@apache.org>.
jonkeane commented on pull request #11592:
URL: https://github.com/apache/arrow/pull/11592#issuecomment-958021000
As for DATE64, as far as I can tell those map back on to dates (even though they have ms precision):
```
> Scalar$create(1L * 1000*60*60*24, int64())$cast(date64())
Scalar
1970-01-02
> Scalar$create(1L * 1000*60*60*24 + 100, int64())$cast(date64())
Scalar
1970-01-02
> Scalar$create(1L * 1000*60*60*24 * 2, int64())$cast(date64())
Scalar
1970-01-03
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org