[GitHub] [arrow] thisisnic commented on a diff in pull request #13163: ARROW-16516: [R] Implement ym() my() and yq() parsers

[GitBox <gi...@apache.org>]

thisisnic commented on code in PR #13163:
URL: https://github.com/apache/arrow/pull/13163#discussion_r874578997


##########
r/R/dplyr-datetime-helpers.R:
##########
@@ -176,7 +191,8 @@ build_formats <- function(orders) {
   }
 
   formats_list <- map(orders, build_format_from_order)
-  purrr::flatten_chr(formats_list)
+  formats <- purrr::flatten_chr(formats_list)

Review Comment:
   Ooh, I didn't know `purrr` had this function, nice!



##########
r/R/dplyr-datetime-helpers.R:
##########
@@ -159,6 +159,21 @@ build_formats <- function(orders) {
   orders <- gsub("[^A-Za-z_]", "", orders)
   orders <- gsub("Y", "y", orders)
 
+  short_orders <- c("ym", "my")
+
+  if (any(orders %in% short_orders)) {
+    orders1 <- setdiff(orders, short_orders)
+    orders2 <- intersect(orders, short_orders)
+    orders2 <- paste0(orders2, "d")
+    orders <- unique(c(orders1, orders2))
+  }
+
+  if (any(orders == "yq")) {
+    orders1 <- setdiff(orders, "yq")
+    orders2 <- "ymd"

Review Comment:
   Again, please add a comment here.



##########
r/R/dplyr-funcs-datetime.R:
##########
@@ -493,11 +493,29 @@ register_bindings_datetime_parsers <- function() {
     # each order is translated into possible formats
     formats <- build_formats(orders)
 
+    x <- x$cast(string())
+
     # make all separators (non-letters and non-numbers) into "-"
     x <- call_binding("gsub", "[^A-Za-z0-9]", "-", x)
     # collapse multiple separators into a single one
     x <- call_binding("gsub", "-{2,}", "-", x)
 
+    # add a day (01) for "ym" and "my" orders
+    augmented_x <- NULL
+    if (any(orders %in% c("ym", "my"))) {
+      augmented_x <- call_binding("paste0", x, "-01")
+    }
+
+    augmented_x2 <- NULL
+    if (any(orders == "yq")) {
+      quarter_x <- call_binding("gsub", "^.*?-", "", x)

Review Comment:
   Please add a brief comment explaining this regex



##########
r/R/dplyr-datetime-helpers.R:
##########
@@ -159,6 +159,21 @@ build_formats <- function(orders) {
   orders <- gsub("[^A-Za-z_]", "", orders)
   orders <- gsub("Y", "y", orders)
 
+  short_orders <- c("ym", "my")
+
+  if (any(orders %in% short_orders)) {

Review Comment:
   Please can you add a brief comment summarising what this block of code is doing?



##########
r/R/dplyr-funcs-datetime.R:
##########
@@ -514,6 +532,36 @@ register_bindings_datetime_parsers <- function() {
       )
     }
 
+    parse_attempt_exp_augmented_x <- list()
+
+    if (!is.null(augmented_x)) {
+      for (i in seq_along(formats)) {
+        parse_attempt_expressions[[i]] <- build_expr(
+          "strptime",
+          augmented_x,
+          options = list(format = formats[[i]], unit = 0L, error_is_null = TRUE)
+        )
+      }
+    }
+
+    parse_attempt_exp_augmented_x2 <- list()
+
+    if (!is.null(augmented_x2)) {
+      for (i in seq_along(formats)) {
+        parse_attempt_expressions[[i]] <- build_expr(
+          "strptime",
+          augmented_x2,
+          options = list(format = formats[[i]], unit = 0L, error_is_null = TRUE)
+        )
+      }
+    }
+
+    parse_attempt_expressions <- c(

Review Comment:
   Brief comment here too wouldn't go amiss



-- 
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