GH-33708: [R] read_csv_arrow()'s timestamp_parsers parameter is a bit light on documentation and doesn't appear to do anything

r/R/csv.R

@@ -118,16 +118,24 @@
 #' `TRUE`, blank rows will not be represented at all. If `FALSE`, they will be
 #' filled with missings.
 #' @param skip Number of lines to skip before reading data.
-#' @param timestamp_parsers User-defined timestamp parsers. If more than one
-#' parser is specified, the CSV conversion logic will try parsing values
-#' starting from the beginning of this vector. Possible values are:
+#' @param timestamp_parsers User-defined timestamp parsers, tried in order
+#' when inferring column types and when converting columns of type
+#' [timestamp()]. Possible values are:
 #'  - `NULL`: the default, which uses the ISO-8601 parser
 #'  - a character vector of [strptime][base::strptime()] parse strings
-#'  - a list of [TimestampParser] objects
+#'  - a list of [TimestampParser] objects and/or parse strings
+#'
+#' Supplying parsers replaces the default ISO-8601 parser rather than adding
+#' to it. If none of the parsers match a value during type inference, the
+#' column is read as a string without error; to get an error instead, specify
+#' the column as a timestamp in `col_types`. These parsers are not used for
+#' date columns.
 #' @param parse_options see [CSV parsing options][csv_parse_options()].
 #' If given, this overrides any
 #' parsing options provided in other arguments (e.g. `delim`, `quote`, etc.).
-#' @param convert_options see [CSV conversion options][csv_convert_options()]
+#' @param convert_options see [CSV conversion options][csv_convert_options()].
+#' If given, this overrides any conversion options provided in other arguments
+#' (e.g. `na`, `col_types`, `timestamp_parsers`, etc.).
 #' @param read_options see [CSV reading options][csv_read_options()]
 #' @param as_data_frame Should the function return a `tibble` (default) or
 #' an Arrow [Table]?
@@ -161,6 +169,16 @@
 #'   col_types = schema(x = timestamp(unit = "us", timezone = "UTC"))
 #' )
 #'
+#' # Parse non-ISO timestamps with `timestamp_parsers`. Supplying parsers
+#' # replaces the default ISO-8601 parser, so include `TimestampParser$create()`
+#' # to keep it as a fallback:
+#' write.csv(
+#'   data.frame(x = c("16/01/2023 19:47", "2023-01-17 08:00:00")),
+#'   file = tf,
+#'   row.names = FALSE
+#' )
+#' read_csv_arrow(tf, timestamp_parsers = list("%d/%m/%Y %H:%M", TimestampParser$create()))
+#'
 #' # Read directly from strings with `I()`
 #' read_csv_arrow(I("x,y\n1,2\n3,4"))
 #' read_delim_arrow(I(c("x y", "1 2", "3 4")), delim = " ")
@@ -201,6 +219,12 @@ read_delim_arrow <- function(
   if (is.null(read_options)) {
     read_options <- readr_to_csv_read_options(skip, col_names)
   }
+  if (!is.null(convert_options) && !is.null(timestamp_parsers)) {
+    rlang::warn(c(
+      "`timestamp_parsers` is ignored when `convert_options` is supplied.",
+      i = "Pass it via `csv_convert_options(timestamp_parsers = ...)` instead."
+    ))
+  }
   if (is.null(convert_options)) {
     convert_options <- readr_to_csv_convert_options(
       na = na,
@@ -535,17 +559,19 @@ csv_read_options <- function(
 #'    columns named in it but not found in the data be included as a column of
 #'    type `null()`? The default (`FALSE`) means that the reader will instead
 #'    raise an error.
-#' - `timestamp_parsers` User-defined timestamp parsers. If more than one
-#'    parser is specified, the CSV conversion logic will try parsing values
-#'    starting from the beginning of this vector. Possible values are
-#'    (a) `NULL`, the default, which uses the ISO-8601 parser;
+#' - `timestamp_parsers` User-defined timestamp parsers, tried in order when
+#'    inferring column types and when converting timestamp columns. Possible
+#'    values are (a) `NULL`, the default, which uses the ISO-8601 parser;
 #'    (b) a character vector of [strptime][base::strptime()] parse strings; or
-#'    (c) a list of [TimestampParser] objects.
+#'    (c) a list of [TimestampParser] objects and/or parse strings. Supplying
+#'    parsers replaces the default ISO-8601 parser; see [read_delim_arrow()]
+#'    for details.
 #' - `decimal_point` Character to use for decimal point in floating point numbers. Default: "."
 #'
 #' `TimestampParser$create()` takes an optional `format` string argument.
 #' See [`strptime()`][base::strptime()] for example syntax.
-#' The default is to use an ISO-8601 format parser.
+#' The default is to use an ISO-8601 format parser, which is useful as a
+#' fallback at the end of a list of `timestamp_parsers`.
 #'
 #' The `CsvWriteOptions$create()` factory method takes the following arguments:
 #' - `include_header` Whether to write an initial header line with column names
@@ -801,13 +827,8 @@ TimestampParser$create <- function(format = NULL) {
 #'    columns named in it but not found in the data be included as a column of
 #'    type `null()`? The default (`FALSE`) means that the reader will instead
 #'    raise an error.
-#' @param timestamp_parsers User-defined timestamp parsers. If more than one
-#'    parser is specified, the CSV conversion logic will try parsing values
-#'    starting from the beginning of this vector. Possible values are
-#'    (a) `NULL`, the default, which uses the ISO-8601 parser;
-#'    (b) a character vector of [strptime][base::strptime()] parse strings; or
-#'    (c) a list of [TimestampParser] objects.
 #' @param decimal_point Character to use for decimal point in floating point numbers.
+#' @inheritParams read_delim_arrow
 #'
 #' @examplesIf arrow_with_dataset()
 #' tf <- tempfile()

r/R/dataset-format.R

@@ -232,6 +232,12 @@ check_csv_file_format_args <- function(args, partitioning = NULL) {
     args$read_options <- list(col_names = args$col_names)
   }
 
+  if (!is.null(args$convert_options) && !is.null(args$timestamp_parsers)) {
+    rlang::warn(c(
+      "`timestamp_parsers` is ignored when `convert_options` is supplied.",
+      i = "Pass it via `csv_convert_options(timestamp_parsers = ...)` instead."
+    ))
+  }
   if (is.null(args$convert_options)) {
     options$convert_options <- do.call(csv_file_format_convert_opts, c(args, list(read_options = options$read_options)))
   } else if (is.list(args$convert_options)) {

r/tests/testthat/test-csv.R

@@ -760,3 +760,45 @@ test_that("altrep columns can roundtrip to table", {
   # we should still be able to turn this into a table
   expect_equal(tbl, as_tibble(arrow_table(new_df)))
 })
+
+test_that("timestamp_parsers during type inference", {
+  tf <- tempfile()
+  on.exit(unlink(tf))
+  writeLines(c("time", "16/01/2023 19:47"), tf)
+  expected <- as.POSIXct("2023-01-16 19:47:00", tz = "UTC")
+
+  # A matching parser is used during type inference
+  df <- read_csv_arrow(tf, timestamp_parsers = "%d/%m/%Y %H:%M")
+  expect_equal(df$time, expected, ignore_attr = "tzone")
+
+  # A non-matching parser falls through to string, without error
+  df <- read_csv_arrow(tf, timestamp_parsers = "%m-%d-%y")
+  expect_type(df$time, "character")
+
+  # Supplying parsers replaces the ISO-8601 default...
+  writeLines(c("time", "16/01/2023 19:47", "2023-01-17 08:00:00"), tf)
+  df <- read_csv_arrow(tf, timestamp_parsers = "%d/%m/%Y %H:%M")
+  expect_type(df$time, "character")
+
+  # ...unless TimestampParser$create() is included as a fallback
+  df <- read_csv_arrow(
+    tf,
+    timestamp_parsers = list("%d/%m/%Y %H:%M", TimestampParser$create())
+  )
+  expect_equal(
+    df$time,
+    as.POSIXct(c("2023-01-16 19:47:00", "2023-01-17 08:00:00"), tz = "UTC"),
+    ignore_attr = "tzone"
+  )
+
+  # timestamp_parsers is ignored, with a warning, when convert_options is supplied
+  expect_warning(
+    df <- read_csv_arrow(
+      tf,
+      convert_options = csv_convert_options(),
+      timestamp_parsers = "%d/%m/%Y %H:%M"
+    ),
+    "`timestamp_parsers` is ignored"
+  )
+  expect_type(df$time, "character")
+})

r/tests/testthat/test-dataset-csv.R

@@ -628,17 +628,22 @@ test_that("open_delim_dataset params passed through to open_dataset", {
   expect_equal(ds$x, c(NA, 1L, NA, NA, 2L, NA, 3L))
 
   # timestamp_parsers
-  skip("GH-33708: timestamp_parsers don't appear to be working properly")
-
   dst_dir <- make_temp_dir()
   dst_file <- file.path(dst_dir, "data.csv")
+  writeLines(c("time", "16/01/2023 19:47"), dst_file)
 
-  df <- data.frame(time = "2023-01-16 19:47:57")
-  write.csv(df, dst_file, row.names = FALSE, quote = FALSE)
-
-  ds <- open_csv_dataset(dst_dir, timestamp_parsers = c(TimestampParser$create(format = "%d-%m-%y"))) |> collect()
+  ds <- open_csv_dataset(dst_dir, timestamp_parsers = "%d/%m/%Y %H:%M") |> collect()
+  expect_equal(ds$time, as.POSIXct("2023-01-16 19:47:00", tz = "UTC"), ignore_attr = "tzone")
 
-  expect_equal(ds$time, "16-01-2023")
+  # timestamp_parsers is ignored, with a warning, when convert_options is supplied
+  expect_warning(
+    open_csv_dataset(
+      dst_dir,
+      convert_options = csv_convert_options(),
+      timestamp_parsers = "%d/%m/%Y %H:%M"
+    ),
+    "`timestamp_parsers` is ignored"
+  )
 })
 
 test_that("CSVReadOptions printing", {