You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "eitsupi (via GitHub)" <gi...@apache.org> on 2024/11/02 14:32:40 UTC
[PR] fix(r): sub-day precision Date should be floored when treated as integer [arrow-nanoarrow]
eitsupi opened a new pull request, #674:
URL: https://github.com/apache/arrow-nanoarrow/pull/674
Fix #665
--
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
Re: [PR] fix(r): sub-day precision Date should be floored when treated as integer [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #674:
URL: https://github.com/apache/arrow-nanoarrow/pull/674#issuecomment-2453752073
One more data point: vctrs coerces to double on `vec_data()`:
``` r
vctr <- as.Date(c(-1L, 0L, 1L))
typeof(vctr)
#> [1] "integer"
typeof(vctrs::vec_data(vctr))
#> [1] "double"
```
In theory we should be able to specify this as a target with `convert_array()`, but it looks like this doesn't work:
``` r
library(nanoarrow)
# Should work but doesn't!
dt <- as_nanoarrow_array(Sys.Date())
convert_array(dt, as.Date(integer()))
#> Error in convert_array.default(array, to, .from_c = TRUE): Can't convert array <date32> to R vector of type Date
```
...all this to say that I'm not sure I'm convinced that the default should be changed, but we can definitely improve this handling! Thank you!
--
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
Re: [PR] fix(r): sub-day precision Date should be floored when treated as integer [arrow-nanoarrow]
Posted by "eitsupi (via GitHub)" <gi...@apache.org>.
eitsupi commented on code in PR #674:
URL: https://github.com/apache/arrow-nanoarrow/pull/674#discussion_r1826896416
##########
r/R/as-array.R:
##########
@@ -262,8 +262,14 @@ as_nanoarrow_array.Date <- function(x, ..., schema = NULL) {
switch(
parsed$type,
date32 = {
+ int_vec <- if (is.integer(x)) {
+ as.integer(x)
Review Comment:
Thinking about it again, I think this is necessary to remove the class (without it, wouldn't we be stuck in an endless loop?).
--
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
Re: [PR] fix(r): sub-day precision Date should be floored when treated as integer [arrow-nanoarrow]
Posted by "eitsupi (via GitHub)" <gi...@apache.org>.
eitsupi commented on PR #674:
URL: https://github.com/apache/arrow-nanoarrow/pull/674#issuecomment-2453307139
> This is to better align with how base R normally handles this (although I think either are OK).
>
> ```r
> typeof(as.Date("2001-02-03"))
> #> [1] "double"
> typeof(Sys.Date())
> #> [1] "double"
> ```
Indeed, it seems that when a number is converted to Date with `as.Date()`, the original numeric type is preserved.
So I think it does not seem surprising that the output of nanoarrow would be an integer.
``` r
typeof(as.Date(1L))
#> [1] "integer"
```
<sup>Created on 2024-11-03 with [reprex v2.1.1](https://reprex.tidyverse.org)</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
Re: [PR] fix(r): sub-day precision Date should be floored when treated as integer [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #674:
URL: https://github.com/apache/arrow-nanoarrow/pull/674
--
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
Re: [PR] fix(r): sub-day precision Date should be floored when treated as integer [arrow-nanoarrow]
Posted by "eitsupi (via GitHub)" <gi...@apache.org>.
eitsupi commented on code in PR #674:
URL: https://github.com/apache/arrow-nanoarrow/pull/674#discussion_r1826579980
##########
r/tests/testthat/test-as-array.R:
##########
@@ -464,7 +464,13 @@ test_that("as_nanoarrow_array() works for Date -> na_date32()", {
expect_identical(as.raw(array$buffers[[1]]), as.raw(0x03))
expect_identical(
as.raw(array$buffers[[2]]),
- as.raw(as_nanoarrow_buffer(c(10957L, 19391L, NA)))
+ as.raw(as_nanoarrow_buffer(c(10957L, 19391L, 0L)))
Review Comment:
I do not know why this change occurred.
(I also do not understand what this test means.)
--
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
Re: [PR] fix(r): sub-day precision Date should be floored when treated as integer [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #674:
URL: https://github.com/apache/arrow-nanoarrow/pull/674#discussion_r1826897998
##########
r/R/as-array.R:
##########
@@ -262,8 +262,14 @@ as_nanoarrow_array.Date <- function(x, ..., schema = NULL) {
switch(
parsed$type,
date32 = {
+ int_vec <- if (is.integer(x)) {
+ as.integer(x)
Review Comment:
Ah, I didn't get that from reading. Either a comment or an `unclass(x)` would work for me so that we don't accidentally simplify this 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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(r): sub-day precision Date should be floored when treated as integer [arrow-nanoarrow]
Posted by "eitsupi (via GitHub)" <gi...@apache.org>.
eitsupi commented on code in PR #674:
URL: https://github.com/apache/arrow-nanoarrow/pull/674#discussion_r1826896416
##########
r/R/as-array.R:
##########
@@ -262,8 +262,14 @@ as_nanoarrow_array.Date <- function(x, ..., schema = NULL) {
switch(
parsed$type,
date32 = {
+ int_vec <- if (is.integer(x)) {
+ as.integer(x)
Review Comment:
Thinking about it again, I think this is necessary to remove the class attribute (without it, wouldn't we be stuck in an endless loop?).
--
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
Re: [PR] fix(r): sub-day precision Date should be floored when treated as integer [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #674:
URL: https://github.com/apache/arrow-nanoarrow/pull/674#discussion_r1826890729
##########
r/R/as-array.R:
##########
@@ -262,8 +262,14 @@ as_nanoarrow_array.Date <- function(x, ..., schema = NULL) {
switch(
parsed$type,
date32 = {
+ int_vec <- if (is.integer(x)) {
+ as.integer(x)
Review Comment:
```suggestion
x
```
--
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