You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "thisisnic (via GitHub)" <gi...@apache.org> on 2023/07/13 10:17:47 UTC
[GitHub] [arrow] thisisnic commented on a diff in pull request #36436: GH-36247: [R] Add write_csv_dataset
thisisnic commented on code in PR #36436:
URL: https://github.com/apache/arrow/pull/36436#discussion_r1262303120
##########
r/R/csv.R:
##########
@@ -512,30 +512,46 @@ CsvReadOptions$create <- function(use_threads = option_use_threads(),
readr_to_csv_write_options <- function(include_header = TRUE,
batch_size = 1024L,
- na = "") {
+ delim = ",",
+ na = "",
+ eol = "\n") {
CsvWriteOptions$create(
include_header = include_header,
batch_size = batch_size,
- null_string = na
+ delimiter = delim,
+ null_string = na,
+ eol = eol
)
}
#' @rdname CsvReadOptions
#' @export
CsvWriteOptions <- R6Class("CsvWriteOptions", inherit = ArrowObject)
-CsvWriteOptions$create <- function(include_header = TRUE, batch_size = 1024L, null_string = "") {
- assert_that(is_integerish(batch_size, n = 1, finite = TRUE), batch_size > 0)
+CsvWriteOptions$create <- function(include_header = TRUE,
+ batch_size = 1024L,
+ delimiter = ",",
+ null_string = "",
+ eol = "\n") {
assert_that(is.logical(include_header))
+
+ assert_that(is_integerish(batch_size, n = 1, finite = TRUE), batch_size > 0)
+
+ assert_that(is.character(delimiter))
+
assert_that(is.character(null_string))
assert_that(!is.na(null_string))
assert_that(length(null_string) == 1)
assert_that(!grepl('"', null_string), msg = "na argument must not contain quote characters.")
+ assert_that(is.character(eol))
Review Comment:
Another comment re the extra spacing. Thanks for adding all these checks though, this will be nice and robust!
##########
r/R/csv.R:
##########
@@ -512,30 +512,46 @@ CsvReadOptions$create <- function(use_threads = option_use_threads(),
readr_to_csv_write_options <- function(include_header = TRUE,
batch_size = 1024L,
- na = "") {
+ delim = ",",
+ na = "",
+ eol = "\n") {
CsvWriteOptions$create(
include_header = include_header,
batch_size = batch_size,
- null_string = na
+ delimiter = delim,
+ null_string = na,
+ eol = eol
)
}
#' @rdname CsvReadOptions
#' @export
CsvWriteOptions <- R6Class("CsvWriteOptions", inherit = ArrowObject)
-CsvWriteOptions$create <- function(include_header = TRUE, batch_size = 1024L, null_string = "") {
- assert_that(is_integerish(batch_size, n = 1, finite = TRUE), batch_size > 0)
+CsvWriteOptions$create <- function(include_header = TRUE,
+ batch_size = 1024L,
+ delimiter = ",",
+ null_string = "",
+ eol = "\n") {
Review Comment:
Would you mind rearranging these so that the previously included options come before the added ones? The reasoning here is that people might be using this function already (as it's exported) without naming the options, so reordering them could constitute a breaking change to their code.
##########
r/R/dataset-format.R:
##########
@@ -76,9 +76,10 @@ FileFormat <- R6Class("FileFormat",
)
FileFormat$create <- function(format, schema = NULL, ...) {
opt_names <- names(list(...))
- if (format %in% c("csv", "text") || any(opt_names %in% c("delim", "delimiter"))) {
+ if (format %in% c("csv", "text", "txt") || any(opt_names %in% c("delim", "delimiter"))) {
CsvFileFormat$create(schema = schema, ...)
} else if (format == "tsv") {
+ # This delimiter argument is ignored.
Review Comment:
Could this be solved by using [`FileFormat$create(format = "tsv", ...)` ](https://arrow.apache.org/docs/dev/r/reference/FileFormat.html) on the line below?
##########
r/R/csv.R:
##########
@@ -512,30 +512,46 @@ CsvReadOptions$create <- function(use_threads = option_use_threads(),
readr_to_csv_write_options <- function(include_header = TRUE,
batch_size = 1024L,
- na = "") {
+ delim = ",",
+ na = "",
+ eol = "\n") {
CsvWriteOptions$create(
include_header = include_header,
batch_size = batch_size,
- null_string = na
+ delimiter = delim,
+ null_string = na,
+ eol = eol
)
}
#' @rdname CsvReadOptions
#' @export
CsvWriteOptions <- R6Class("CsvWriteOptions", inherit = ArrowObject)
-CsvWriteOptions$create <- function(include_header = TRUE, batch_size = 1024L, null_string = "") {
- assert_that(is_integerish(batch_size, n = 1, finite = TRUE), batch_size > 0)
+CsvWriteOptions$create <- function(include_header = TRUE,
+ batch_size = 1024L,
+ delimiter = ",",
+ null_string = "",
+ eol = "\n") {
assert_that(is.logical(include_header))
+
+ assert_that(is_integerish(batch_size, n = 1, finite = TRUE), batch_size > 0)
+
+ assert_that(is.character(delimiter))
+
Review Comment:
Really minor, but would you mind removing the extra spaces here?
##########
r/R/dataset-write.R:
##########
@@ -178,12 +178,32 @@ write_dataset <- function(dataset,
}
path_and_fs <- get_path_and_filesystem(path)
+
+ dots <- list(...)
+ if (format %in% c("csv", "tsv") && any(c("delimiter", "delim") %in% names(dots))) {
+ stop("Do not set a delimiter for csv or tsv formats.")
+ }
Review Comment:
I like your reasoning here, though to note, when I did a search for "alternative delimiter for csv", it appears that, despite the name "comma separated values", it's not uncommon for csv files to use alternative delimiters.
What I'd recommend is either:
1. let's let people specify CSV with an alternative delimiter and go with the delimiter they specify, or
2. we update this text in this error message to use "must" phrasing as per [the tidyverse style guide](https://style.tidyverse.org/error-messages.html), and recommend they specify the format as "text" instead of whatever format they've specified (and output that too).
I'll leave the design decision up to you - let me know what you think is best!
--
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