ARROW-16415: [R] Update `strptime` binding signature with the `tz` argument by dragosmg · Pull Request #13190 · apache/arrow · GitHub

This PR enables the tz argument for the strptime binding by using the assume_timezone compute function.

Current behaviour:

library(arrow, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)

test_df <- tibble::tibble(
    timestamp_naive_string = c("2018-10-07 19:04:05", NA)
)

test_df %>% 
    arrow_table() %>% 
    mutate(
        timestamp = 
            strptime(
                timestamp_naive_string, 
                format = "%Y-%m-%d %H:%M:%S", 
                tz = "Pacific/Marquesas"
            )
    ) %>%
    collect()
#> Warning: In strptime(timestamp_naive_string, format = "%Y-%m-%d %H:%M:%S", ...,
#> Time zone argument not supported in Arrow; pulling data into R
#> # A tibble: 2 × 2
#>   timestamp_naive_string timestamp          
#>   <chr>                  <dttm>             
#> 1 2018-10-07 19:04:05    2018-10-07 19:04:05
#> 2 <NA>                   NA

Created on 2022-05-18 by the reprex package (v2.0.1)

Future/desired behaviour:

library(arrow, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)

test_df <- tibble::tibble(
  timestamp_naive_string = c("2018-10-07 19:04:05", NA)
)

a <- test_df %>% 
  arrow_table() %>% 
  mutate(
    timestamp = 
      strptime(
        timestamp_naive_string, 
        format = "%Y-%m-%d %H:%M:%S", 
        tz = "Pacific/Marquesas"
      )
  ) %>%
  collect()

a
#> # A tibble: 2 × 2
#>   timestamp_naive_string timestamp          
#>   <chr>                  <dttm>             
#> 1 2018-10-07 19:04:05    2018-10-07 19:04:05
#> 2 <NA>                   NA
attributes(a$timestamp)
#> $class
#> [1] "POSIXlt" "POSIXt" 
#> 
#> $tzone
#> [1] "Pacific/Marquesas"

Created on 2022-05-19 by the reprex package (v2.0.1)

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

I think you might have some issues where the default argument of tz in strptime() in R is different to the one here. I tried running this test I wrote, but got an error:

> t_string %>%
+       arrow_table() %>%
+       mutate(
+         x = strptime(x, format = "%Y-%m-%d %H:%M:%S", tz = "")
+       ) %>%
+       collect() -> out
Error in `collect()`:
! Invalid: Cannot locate timezone '':  not found in timezone database
/home/nic2/arrow/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc:128  LocateZone(options.timezone)
/home/nic2/arrow/cpp/src/arrow/compute/exec.cc:701  kernel_->exec(kernel_ctx_, batch, &out)
/home/nic2/arrow/cpp/src/arrow/compute/exec.cc:642  ExecuteBatch(batch, listener)
/home/nic2/arrow/cpp/src/arrow/compute/exec/expression.cc:597  executor->Execute(arguments, &listener)
/home/nic2/arrow/cpp/src/arrow/compute/exec/project_node.cc:90  ExecuteScalarExpression(simplified_expr, target, plan()->exec_context())
/home/nic2/arrow/cpp/src/arrow/compute/exec/exec_plan.cc:467  iterator_.Next()
/home/nic2/arrow/cpp/src/arrow/record_batch.cc:337  ReadNext(&batch)
/home/nic2/arrow/cpp/src/arrow/record_batch.cc:351  ToRecordBatches()

Other than the comment it'd be great to see added, I think this is good to go. @jonkeane - mind taking a look when you have a chance, in case I've missed anything?

This is looking good — a comment about an additional test + a few more comments that would be helpful

Looks good, thanks for sticking with it

…Hub issue numbers (#34260) Rewrite the Jira issue numbers to the GitHub issue numbers, so that the GitHub issue numbers are automatically linked to the issues by pkgdown's auto-linking feature. Issue numbers have been rewritten based on the following correspondence. Also, the pkgdown settings have been changed and updated to link to GitHub. I generated the Changelog page using the `pkgdown::build_news()` function and verified that the links work correctly. --- ARROW-6338 #5198 ARROW-6364 #5201 ARROW-6323 #5169 ARROW-6278 #5141 ARROW-6360 #5329 ARROW-6533 #5450 ARROW-6348 #5223 ARROW-6337 #5399 ARROW-10850 #9128 ARROW-10624 #9092 ARROW-10386 #8549 ARROW-6994 #23308 ARROW-12774 #10320 ARROW-12670 #10287 ARROW-16828 #13484 ARROW-14989 #13482 ARROW-16977 #13514 ARROW-13404 #10999 ARROW-16887 #13601 ARROW-15906 #13206 ARROW-15280 #13171 ARROW-16144 #13183 ARROW-16511 #13105 ARROW-16085 #13088 ARROW-16715 #13555 ARROW-16268 #13550 ARROW-16700 #13518 ARROW-16807 #13583 ARROW-16871 #13517 ARROW-16415 #13190 ARROW-14821 #12154 ARROW-16439 #13174 ARROW-16394 #13118 ARROW-16516 #13163 ARROW-16395 #13627 ARROW-14848 #12589 ARROW-16407 #13196 ARROW-16653 #13506 ARROW-14575 #13160 ARROW-15271 #13170 ARROW-16703 #13650 ARROW-16444 #13397 ARROW-15016 #13541 ARROW-16776 #13563 ARROW-15622 #13090 ARROW-18131 #14484 ARROW-18305 #14581 ARROW-18285 #14615 * Closes: #33631 Authored-by: SHIMA Tatsuya <ts1s1andn@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

adding tz support to strptime + updated unit tests

aaa43a5

github-actions Bot added the Component: R label May 18, 2022

dragosmg changed the title ARROW-16415: [R] Update strptime bindings to use tz ARROW-16415: [R] Update strptime binding signature with the tz argument May 18, 2022

dragosmg marked this pull request as ready for review May 18, 2022 20:53

thisisnic requested changes May 25, 2022

View reviewed changes

dragosmg added 2 commits May 25, 2022 13:40

set default value for tz to match base::strptime()

2619723

update unit tests

f9c29a1

dragosmg requested a review from thisisnic May 25, 2022 12:41

dragosmg added 3 commits May 25, 2022 14:37

bump ci

b42453e

bump ci

c27dbf9

bump ci

5525acc

thisisnic reviewed May 26, 2022

View reviewed changes

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

thisisnic approved these changes May 26, 2022

View reviewed changes

thisisnic requested a review from jonkeane May 26, 2022 11:11

comment + record_batch() in one of the tests

e34f552

jonkeane requested changes Jun 1, 2022

View reviewed changes

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

Comment thread r/R/dplyr-funcs-datetime.R

added comment on the use of assume_timezone

8e216aa

dragosmg requested a review from jonkeane June 6, 2022 09:42

jonkeane reviewed Jun 6, 2022

View reviewed changes

Comment thread r/R/dplyr-funcs-datetime.R Outdated

dragosmg added 2 commits June 7, 2022 09:24

expanded on why assume_timezone is needed

ee73795

improved comments and renamed one of the test tibbles for more clarity

f0c3323

dragosmg requested a review from jonkeane June 7, 2022 09:02

forgot about one t_stamp

ca4e902

jonkeane reviewed Jun 7, 2022

View reviewed changes

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

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

90ce31b

jonkeane approved these changes Jun 7, 2022

View reviewed changes

jonkeane merged commit 3c1caea into apache:master Jun 7, 2022

asfimport mentioned this pull request Jun 7, 2022

[R] Update strptime bindings to use tz #31791

Closed

eitsupi mentioned this pull request Feb 19, 2023

GH-33631: [R] Rewrite Jira ticket numbers in pkgdown documents to GitHub issue numbers #34260

Merged

ARROW-16415: [R] Update `strptime` binding signature with the `tz` argument by dragosmg · Pull Request #13190 · apache/arrow · GitHub

Uh oh!

ARROW-16415: [R] Update strptime binding signature with the tz argument - #13190

ARROW-16415: [R] Update strptime binding signature with the tz argument#13190
jonkeane merged 12 commits into
apache:masterfrom
dragosmg:strptime_tz_update

dragosmg commented May 18, 2022 •
edited
Loading

Uh oh!

Uh oh!

github-actions Bot commented May 18, 2022

Uh oh!

thisisnic left a comment

Uh oh!

Uh oh!

thisisnic left a comment

Uh oh!

jonkeane left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonkeane left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ARROW-16415: [R] Update `strptime` binding signature with the `tz` argument by dragosmg · Pull Request #13190 · apache/arrow · GitHub

Uh oh!

ARROW-16415: [R] Update strptime binding signature with the tz argument - #13190

ARROW-16415: [R] Update strptime binding signature with the tz argument#13190jonkeane merged 12 commits intoapache:masterapache/arrow:masterfrom dragosmg:strptime_tz_updatedragosmg/arrow:strptime_tz_updateCopy head branch name to clipboard

Conversation

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

Uh oh!

Uh oh!

github-actions Bot commented May 18, 2022

Uh oh!

thisisnic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thisisnic left a comment

Choose a reason for hiding this comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ARROW-16415: [R] Update strptime binding signature with the tz argument#13190
jonkeane merged 12 commits into
apache:masterfrom
dragosmg:strptime_tz_update

dragosmg commented May 18, 2022 •
edited
Loading