ARROW-14808 [R] Implement bindings for `lubridate::date()` #12433

dragosmg · 2022-02-15T17:07:24Z

The following will be supported in arrow (base::as.Date() included to show the difference to date(), but not supported):

library(dplyr)
library(lubridate)

df <- tibble(a = as.POSIXct("2012-03-26 23:12:13", tz = "America/New_York"))

df %>%
  mutate(
    a_date = date(a),
    a_date_base = as.Date(a))
#> # A tibble: 1 × 3
#>   a                   a_date     a_date_base
#>   <dttm>              <date>     <date>     
#> 1 2012-03-26 23:12:13 2012-03-26 2012-03-27

library(arrow)
library(dplyr)
library(lubridate)

df <- tibble(a = as.POSIXct("2012-03-26 23:12:13", tz = "America/New_York"))
df %>% 
  arrow_table() %>% 
  mutate(
    a_date = date(a)
  ) %>% 
  collect()
#> # A tibble: 1 × 2
#>   a                   a_date    
#>   <dttm>              <date>    
#> 1 2012-03-26 23:12:13 2012-03-26

^{Created on 2022-02-15 by the reprex package (v2.0.1)}

github-actions · 2022-02-15T17:07:57Z

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

jonkeane

Looking good — a few questions for you

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

r/R/dplyr-funcs-datetime.R

jonkeane

One comment + suggestion.

Do we have a ticket to do as.Date separately? Could we do it with this one since it is very very similar?

r/R/dplyr-funcs-datetime.R

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

jonkeane

A few comments

r/R/dplyr-funcs-type.R

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

jonkeane

This is making progress! I didn't quite realize that as.Date() would be as much of a can of worms as it's been — but that's ok, I think it is useful additional functionality!

In the as.Date() implementation it might be nice to restructure the flow so that you have one call at the end to build_expr("cast", intermediate, options = cast_options(to_type = date32())) after having built up intermediate getting things into the right format and then do the cast-to-date (I put intermediate here but that's probably not a good name for it, maybe "just" x like we do elsewhere — I trust you!).

r/R/dplyr-funcs-type.R

dragosmg · 2022-02-28T17:43:03Z

I have created an umbrella issue (ARROW-15805) for the possible improvements to the as.Date() binding + ARROW-15800 to implement as_date().

jonkeane

This is looking good, a few more questions. I didn't realize just how much extra scope as.Date() would add — though I think this is good functionality to add, so that's ok, but don't feel dispirited this is taking longer!

r/R/dplyr-funcs-type.R

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

r/R/dplyr-funcs-type.R

jonkeane

Still getting there, most of the code changes I proposed are attempts to avoid some of the nesting in if/elses. Though I also do think we can support as.Date() with any timezone which will simplify + remove some of the tests down below catching that error.

r/R/dplyr-funcs-type.R

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

r/tests/testthat/test-dplyr-funcs-type.R

dragosmg · 2022-03-02T13:18:24Z

Thanks @jonkeane. Would you mind having another look?

…character, but not an `Expression`

…tolerance)

…k for `origin` at the top of the numeric block

…n + simplified some unit tests

jonkeane

This is fantastic, thank you for all of the pushes. This is incredibly close — one minor comment about an additional comment and one suggestion to use compare_dplyr_binding for testing the warning flow "not supported in arrow -> pulling into R". It's functionally the same, but follows the pattern we use elsewhere.

I'm happy to merge even without these, but these two would add a bit of polish + fit

r/R/dplyr-funcs-type.R

r/tests/testthat/test-dplyr-funcs-type.R

r/R/dplyr-funcs-type.R

jonkeane

Once the CI is green, I'll merge.

Thank you again for all of the work you did on this!

dragosmg · 2022-03-03T16:32:39Z

Thanks for your patience and support 😉

ursabot · 2022-03-03T17:51:21Z

Benchmark runs are scheduled for baseline = ce46c1a and contender = 9719eae. 9719eae is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.13% ⬆️0.0%] test-mac-arm
[Failed ⬇️3.21% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.26% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. 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

`build_expr()` handles the conversion of the inputs to `Array` or `Scalar`. #12433 (comment) Closes #12563 from dragosmg/build_expr_in_as_methods Authored-by: Dragoș Moldovan-Grünfeld <dragos.mold@gmail.com> Signed-off-by: Jonathan Keane <jkeane@gmail.com>

github-actions bot added the Component: R label Feb 15, 2022

dragosmg marked this pull request as draft February 15, 2022 17:26

dragosmg marked this pull request as ready for review February 15, 2022 17:30

dragosmg marked this pull request as draft February 15, 2022 17:45

dragosmg marked this pull request as ready for review February 15, 2022 19:36

jonkeane requested changes Feb 19, 2022

View reviewed changes

r/tests/testthat/test-dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

r/R/dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

dragosmg requested a review from jonkeane February 22, 2022 09:17

dragosmg force-pushed the lubridate_date branch 2 times, most recently from 57ce8c9 to 2d72983 Compare February 23, 2022 14:27

jonkeane requested changes Feb 23, 2022

View reviewed changes

r/R/dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

dragosmg requested a review from jonkeane February 24, 2022 12:45

dragosmg commented Feb 24, 2022

View reviewed changes

r/tests/testthat/test-dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

jonkeane requested changes Feb 24, 2022

View reviewed changes

r/R/dplyr-funcs-type.R Outdated Show resolved Hide resolved

r/tests/testthat/test-dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

dragosmg requested a review from jonkeane February 25, 2022 15:39

jonkeane requested changes Feb 25, 2022

View reviewed changes

r/R/dplyr-funcs-type.R Outdated Show resolved Hide resolved

r/R/dplyr-funcs-type.R Outdated Show resolved Hide resolved

r/R/dplyr-funcs-type.R Outdated Show resolved Hide resolved

dragosmg requested a review from jonkeane February 28, 2022 17:43

jonkeane requested changes Mar 1, 2022

View reviewed changes

dragosmg requested a review from jonkeane March 1, 2022 13:27

jonkeane requested changes Mar 1, 2022

View reviewed changes

dragosmg requested a review from jonkeane March 2, 2022 12:10

dragosmg added 7 commits March 2, 2022 14:44

added binding for date() + unit test + NEWS bullet point

da4158a

namespace the call to date()

1bce2a8

remove namespacing for date() inside the mutate() context

3ea6beb

trying to mask base::date()

96c6b3f

test 2

27cf385

test 2 for the date() masking issue

4ef422d

added comment on namespacing

35ddfec

dragosmg added 15 commits March 2, 2022 14:44

added tryFormats arg and handling for situations in which x is a …

ad49d2c

…character, but not an `Expression`

figure out which one Windows doesn't like

2c26ef7

use interim step + skip test involving tzdb on Windows

9b6a9e2

update NEWS

4105b82

moved unit test to the success block

6e1be4e

remove the interim_x object and the additional step it introduced

47f8119

float -> integer with safe = FALSE & 2 additional unit tests (with …

0b8cfe3

…tolerance)

improved comments

2be4e31

switch back to floor

a280f45

let build_expr() handle conversion to Expression

81e9add

simplify building the strptime expression (unit = O)

af3d5cf

unit tests updates

d09bee8

simplified format vs tryFormats implementation and moved the chec…

5da34fb

…k for `origin` at the top of the numeric block

moved arg checking at the top and reorganised the body of the functio…

4f9d0c1

…n + simplified some unit tests

removed redundant unit tests

47587a9

dragosmg force-pushed the lubridate_date branch from 6ec5bb0 to 47587a9 Compare March 2, 2022 14:45

dragosmg added 4 commits March 2, 2022 15:29

Merge branch 'master' into lubridate_date

41e604f

support other timezones than "UTC" and update tests to reflect that

fc7667c

update NEWS

036e349

remove suport for double and corresponding unit tests

5788fd5

jonkeane approved these changes Mar 3, 2022

View reviewed changes

r/R/dplyr-funcs-type.R Show resolved Hide resolved

r/R/dplyr-funcs-type.R Show resolved Hide resolved

r/tests/testthat/test-dplyr-funcs-type.R Show resolved Hide resolved

r/R/dplyr-funcs-type.R Show resolved Hide resolved

dragosmg added 2 commits March 3, 2022 15:46

use compare_dplyr_binding() to test pull back to R

a549f4c

added a couple of comments to improve quality

1a9bbfd

jonkeane approved these changes Mar 3, 2022

View reviewed changes

jonkeane closed this in 9719eae Mar 3, 2022

dragosmg deleted the lubridate_date branch March 3, 2022 18:15

dragosmg mentioned this pull request Mar 4, 2022

ARROW-15775 [R] Clean up as.* methods to use build_expr() #12563

Closed

ARROW-14808 [R] Implement bindings for lubridate::date() #12433

ARROW-14808 [R] Implement bindings for lubridate::date() #12433

Uh oh!

Conversation

dragosmg commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 15, 2022

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dragosmg commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dragosmg commented Mar 2, 2022

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

dragosmg commented Mar 3, 2022

Uh oh!

ursabot commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ARROW-14808 [R] Implement bindings for `lubridate::date()` #12433

ARROW-14808 [R] Implement bindings for `lubridate::date()` #12433

dragosmg commented Feb 15, 2022 •

edited

Loading

dragosmg commented Feb 28, 2022 •

edited

Loading

ursabot commented Mar 3, 2022 •

edited

Loading