paleolimbot commented on code in PR #33614:
URL: https://github.com/apache/arrow/pull/33614#discussion_r1072231211
##########
r/tests/testthat/test-dataset-csv.R:
##########
@@ -476,3 +476,87 @@ test_that("CSV reading/parsing/convert options can be
passed in as lists", {
expect_equal(ds1, ds2)
})
+
+test_that("open_delim_dataset params passed through to open_dataset", {
+ ds <- open_delim_dataset(csv_dir, delim = ",", partitioning = "part")
+ expect_r6_class(ds$format, "CsvFileFormat")
+ expect_r6_class(ds$filesystem, "LocalFileSystem")
+ expect_identical(names(ds), c(names(df1), "part"))
+ expect_identical(dim(ds), c(20L, 7L))
+
+ # quote
+ df <- data.frame(a = c(1, 2), b = c("'abc'", "'def'"))
+ dst_dir <- make_temp_dir()
+ dst_file <- file.path(dst_dir, "data.csv")
+ write.table(df, sep = ",", dst_file, row.names = FALSE, quote = FALSE)
+
+ ds_quote <- open_csv_dataset(dst_dir, quote = "'") %>% collect()
+ expect_equal(ds_quote$b, c("abc", "def"))
+
+ # na
+ ds <- open_csv_dataset(csv_dir, partitioning = "part", na = c("", "NA",
"FALSE")) %>% collect()
+ expect_identical(ds$lgl, c(
+ TRUE, NA, NA, TRUE, NA, TRUE, NA, NA, TRUE, NA, TRUE, NA, NA,
+ TRUE, NA, TRUE, NA, NA, TRUE, NA
+ ))
+
+ # col_names and skip
+ ds <- open_csv_dataset(
+ csv_dir,
+ partitioning = "part",
+ col_names = paste0("col_", 1:6),
+ skip = 1
+ ) %>% collect()
+
+ expect_named(ds, c("col_1", "col_2", "col_3", "col_4", "col_5", "col_6",
"part"))
+ expect_equal(nrow(ds), 20)
+
+ # col_types
+ df <- data.frame(a = c(1, NA, 2), b = c("'abc'", NA, "'def'"))
+ dst_dir <- make_temp_dir()
+ dst_file <- file.path(dst_dir, "data.csv")
+ write.table(df, sep = ",", dst_file, row.names = FALSE, quote = FALSE)
+
+ data_schema <- schema(a = string(), b = string())
+ ds_strings <- open_csv_dataset(dst_dir, col_types = data_schema)
+ expect_equal(ds_strings$schema, schema(a = string(), b = string()))
+
+ # skip_empty_rows
+ tf <- tempfile()
+ writeLines('"x"\n"y"\nNA\nNA\n"NULL"\n\n\n', tf)
Review Comment:
I like this better than the `write.table()` you do below/above...it's easier
to see what the csv file that's getting parsed is.
##########
r/R/dataset.R:
##########
@@ -228,6 +228,131 @@ open_dataset <- function(sources,
)
}
+#' Open a multi-file dataset of CSV or other delimiter-separated format
+#'
+#' A wrapper around [open_dataset] which explicitly includes parameters
mirroring [read_csv_arrow()],
+#' [read_delim_arrow()], and [read_tsv_arrow()] to allows for easy switching
between functions
+#' for opening single files and functions for opening datasets.
+#'
+#' @inheritParams open_dataset
+#' @inheritParams read_delim_arrow
+#'
+#' @section Options currently supported by [read_delim_arrow()] which are not
supported here:
+#' * `file` (instead, please specify files in `sources`)
+#' * `col_select` (instead, subset columns after dataset creation)
+#' * `quoted_na`
+#' * `as_data_frame` (instead, convert to data frame after dataset creation)
+#' * `parse_options`
+#'
+#' @examplesIf arrow_with_dataset()
+#' # Set up directory for examples
+#' tf <- tempfile()
+#' dir.create(tf)
+#' on.exit(unlink(tf))
+#' df <- data.frame(x = c("1", "2", "NULL"))
+#'
+#' file_path <- file.path(tf, "file1.txt")
+#' write.table(df, file_path, sep = ",", row.names = FALSE)
+#'
+#' read_csv_arrow(file_path, na = c("", "NA", "NULL"), col_names = "y", skip =
1)
+#' open_csv_dataset(file_path, na = c("", "NA", "NULL"), col_names = "y", skip
= 1)
+#'
+#' @seealso [open_dataset()]
+#' @export
+open_delim_dataset <- function(sources,
+ schema = NULL,
+ partitioning = hive_partition(),
+ hive_style = NA,
+ unify_schemas = NULL,
+ factory_options = list(),
+ delim = ",",
+ quote = "\"",
+ escape_double = TRUE,
+ escape_backslash = FALSE,
+ col_names = TRUE,
+ col_types = NULL,
+ na = c("", "NA"),
+ skip_empty_rows = TRUE,
+ skip = 0L,
+ convert_options = NULL,
+ read_options = NULL,
+ timestamp_parsers = NULL) {
+
+ open_dataset(
+ sources = sources,
+ schema = schema,
+ partitioning = partitioning,
+ hive_style = hive_style,
+ unify_schemas = unify_schemas,
+ factory_options = factory_options,
+ format = "text",
+ delim = delim,
+ quote = quote,
+ escape_double = escape_double,
+ escape_backslash = escape_backslash,
+ col_names = col_names,
+ col_types = col_types,
+ na = na,
+ skip_empty_rows = skip_empty_rows,
+ skip = skip,
+ convert_options = convert_options,
+ read_options = read_options,
+ timestamp_parsers = timestamp_parsers
+ )
+}
+
+#' @rdname open_delim_dataset
+#' @export
+open_csv_dataset <- function(sources,
+ schema = NULL,
+ partitioning = hive_partition(),
+ hive_style = NA,
+ unify_schemas = NULL,
+ factory_options = list(),
+ quote = "\"",
+ escape_double = TRUE,
+ escape_backslash = FALSE,
+ col_names = TRUE,
+ col_types = NULL,
+ na = c("", "NA"),
+ skip_empty_rows = TRUE,
+ skip = 0L,
+ convert_options = NULL,
+ read_options = NULL,
+ timestamp_parsers = NULL) {
+ mc <- match.call()
+ mc$delim <- ","
+ mc[[1]] <- get("open_delim_dataset", envir = asNamespace("arrow"))
+ eval.parent(mc)
Review Comment:
This feels too magical to me...is there a reason that
```r
open_csv_dataset <- function(...) {
open_delim_dataset(..., delim = ",")
}
```
wouldn't work? (That would also avoid repeating the default argument values)
##########
r/R/dataset-format.R:
##########
@@ -228,9 +225,32 @@ check_csv_file_format_args <- function(...) {
call. = FALSE
)
}
+}
+
+get_opt_names <- function(args) {
+ opt_names <- names(args)
+
+ # extract names of parse_options, read_options, and convert_options
+ if ("parse_options" %in% names(args) && is.list(args[["parse_options"]])) {
Review Comment:
What happens if `args[["parse_options"]]` is not a list? (Probably should be
in the comments since I can't tell by reading this what it should or should not
be)
##########
r/R/dataset.R:
##########
@@ -228,6 +228,131 @@ open_dataset <- function(sources,
)
}
+#' Open a multi-file dataset of CSV or other delimiter-separated format
+#'
+#' A wrapper around [open_dataset] which explicitly includes parameters
mirroring [read_csv_arrow()],
+#' [read_delim_arrow()], and [read_tsv_arrow()] to allows for easy switching
between functions
+#' for opening single files and functions for opening datasets.
+#'
+#' @inheritParams open_dataset
+#' @inheritParams read_delim_arrow
+#'
+#' @section Options currently supported by [read_delim_arrow()] which are not
supported here:
+#' * `file` (instead, please specify files in `sources`)
+#' * `col_select` (instead, subset columns after dataset creation)
+#' * `quoted_na`
+#' * `as_data_frame` (instead, convert to data frame after dataset creation)
+#' * `parse_options`
+#'
+#' @examplesIf arrow_with_dataset()
+#' # Set up directory for examples
+#' tf <- tempfile()
+#' dir.create(tf)
+#' on.exit(unlink(tf))
Review Comment:
I don't know if `on.exit()` works the way you think it will in an
example...my guess is that it won't work if there are any `on.exit()`s in any
other examples. I would just put `unlink(tf)` at the bottom of your example.
##########
r/R/dataset.R:
##########
@@ -228,6 +228,131 @@ open_dataset <- function(sources,
)
}
+#' Open a multi-file dataset of CSV or other delimiter-separated format
+#'
+#' A wrapper around [open_dataset] which explicitly includes parameters
mirroring [read_csv_arrow()],
+#' [read_delim_arrow()], and [read_tsv_arrow()] to allows for easy switching
between functions
+#' for opening single files and functions for opening datasets.
+#'
+#' @inheritParams open_dataset
+#' @inheritParams read_delim_arrow
+#'
+#' @section Options currently supported by [read_delim_arrow()] which are not
supported here:
+#' * `file` (instead, please specify files in `sources`)
+#' * `col_select` (instead, subset columns after dataset creation)
+#' * `quoted_na`
+#' * `as_data_frame` (instead, convert to data frame after dataset creation)
+#' * `parse_options`
+#'
+#' @examplesIf arrow_with_dataset()
Review Comment:
The other example block you added has a skip for Windows...do you need that
here, too? (Or can the skip for windows on the other one be removed?)
##########
r/tests/testthat/test-dataset-csv.R:
##########
@@ -476,3 +476,87 @@ test_that("CSV reading/parsing/convert options can be
passed in as lists", {
expect_equal(ds1, ds2)
})
+
+test_that("open_delim_dataset params passed through to open_dataset", {
+ ds <- open_delim_dataset(csv_dir, delim = ",", partitioning = "part")
+ expect_r6_class(ds$format, "CsvFileFormat")
+ expect_r6_class(ds$filesystem, "LocalFileSystem")
+ expect_identical(names(ds), c(names(df1), "part"))
+ expect_identical(dim(ds), c(20L, 7L))
+
+ # quote
+ df <- data.frame(a = c(1, 2), b = c("'abc'", "'def'"))
+ dst_dir <- make_temp_dir()
+ dst_file <- file.path(dst_dir, "data.csv")
+ write.table(df, sep = ",", dst_file, row.names = FALSE, quote = FALSE)
+
+ ds_quote <- open_csv_dataset(dst_dir, quote = "'") %>% collect()
+ expect_equal(ds_quote$b, c("abc", "def"))
+
+ # na
+ ds <- open_csv_dataset(csv_dir, partitioning = "part", na = c("", "NA",
"FALSE")) %>% collect()
+ expect_identical(ds$lgl, c(
+ TRUE, NA, NA, TRUE, NA, TRUE, NA, NA, TRUE, NA, TRUE, NA, NA,
+ TRUE, NA, TRUE, NA, NA, TRUE, NA
+ ))
+
+ # col_names and skip
+ ds <- open_csv_dataset(
+ csv_dir,
+ partitioning = "part",
+ col_names = paste0("col_", 1:6),
+ skip = 1
+ ) %>% collect()
+
+ expect_named(ds, c("col_1", "col_2", "col_3", "col_4", "col_5", "col_6",
"part"))
+ expect_equal(nrow(ds), 20)
+
+ # col_types
+ df <- data.frame(a = c(1, NA, 2), b = c("'abc'", NA, "'def'"))
+ dst_dir <- make_temp_dir()
+ dst_file <- file.path(dst_dir, "data.csv")
+ write.table(df, sep = ",", dst_file, row.names = FALSE, quote = FALSE)
+
+ data_schema <- schema(a = string(), b = string())
+ ds_strings <- open_csv_dataset(dst_dir, col_types = data_schema)
+ expect_equal(ds_strings$schema, schema(a = string(), b = string()))
+
+ # skip_empty_rows
+ tf <- tempfile()
+ writeLines('"x"\n"y"\nNA\nNA\n"NULL"\n\n\n', tf)
+
+ ds <- open_csv_dataset(tf, skip_empty_rows = FALSE) %>% collect()
+ expect_equal(nrow(ds), 7)
+
+ # timestamp_parsers
+ df <- data.frame(time = "2023-01-16 19:47:57")
+ dst_dir <- make_temp_dir()
+ dst_file <- file.path(dst_dir, "data.csv")
+ write.table(df, sep = ",", dst_file, row.names = FALSE, quote = FALSE)
+
+ ds <- open_csv_dataset(dst_dir, timestamp_parsers =
c(TimestampParser$create(format = "%d-%m-%y"))) %>% collect()
+
+ # GH-33708: timestamp_parsers don't appear to be working properly
+ expect_error(
+ expect_equal(ds$time, "16-01-2023")
+ )
Review Comment:
What is the expected error message?
##########
r/R/dataset.R:
##########
@@ -228,6 +228,131 @@ open_dataset <- function(sources,
)
}
+#' Open a multi-file dataset of CSV or other delimiter-separated format
+#'
+#' A wrapper around [open_dataset] which explicitly includes parameters
mirroring [read_csv_arrow()],
+#' [read_delim_arrow()], and [read_tsv_arrow()] to allows for easy switching
between functions
+#' for opening single files and functions for opening datasets.
+#'
+#' @inheritParams open_dataset
+#' @inheritParams read_delim_arrow
+#'
+#' @section Options currently supported by [read_delim_arrow()] which are not
supported here:
+#' * `file` (instead, please specify files in `sources`)
+#' * `col_select` (instead, subset columns after dataset creation)
+#' * `quoted_na`
+#' * `as_data_frame` (instead, convert to data frame after dataset creation)
+#' * `parse_options`
+#'
+#' @examplesIf arrow_with_dataset()
+#' # Set up directory for examples
+#' tf <- tempfile()
+#' dir.create(tf)
+#' on.exit(unlink(tf))
+#' df <- data.frame(x = c("1", "2", "NULL"))
+#'
+#' file_path <- file.path(tf, "file1.txt")
+#' write.table(df, file_path, sep = ",", row.names = FALSE)
+#'
+#' read_csv_arrow(file_path, na = c("", "NA", "NULL"), col_names = "y", skip =
1)
+#' open_csv_dataset(file_path, na = c("", "NA", "NULL"), col_names = "y", skip
= 1)
+#'
+#' @seealso [open_dataset()]
+#' @export
+open_delim_dataset <- function(sources,
+ schema = NULL,
Review Comment:
Indent to line up to `function(`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
