thisisnic commented on a change in pull request #12083:
URL: https://github.com/apache/arrow/pull/12083#discussion_r790663925
##########
File path: r/tests/testthat/test-dataset-csv.R
##########
@@ -299,3 +299,21 @@ test_that("open_dataset() deals with BOMs
(byte-order-marks) correctly", {
tibble(a = c(1, 3), b = c(2, 4))
)
})
+
+test_that("Error if read_options$column_names and schema-names differ
(ARROW-14744)", {
+
+ dst_dir <- make_temp_dir()
+ dst_file <- file.path(dst_dir, "file.csv")
+ df <- df1[, c("int", "dbl")]
+ write.csv(df, dst_file, row.names = FALSE, quote = FALSE)
+
+ # Mismatch of column names vs schema given via read_options should raise an
error
+ opts <- CsvReadOptions$create(column_names = c("i", "d"))
+ schema <- schema(int = int32(), dbl = float64())
Review comment:
This is slightly unusual spacing here, and I notice that the linter is
complaining about a "Missing terminal newline" a bit further down; would you
mind running the styler on the files you have changed?
Instructions here:
https://arrow.apache.org/docs/dev/r/articles/developers/workflow.html
##########
File path: r/tests/testthat/test-dataset-csv.R
##########
@@ -299,3 +299,21 @@ test_that("open_dataset() deals with BOMs
(byte-order-marks) correctly", {
tibble(a = c(1, 3), b = c(2, 4))
)
})
+
+test_that("Error if read_options$column_names and schema-names differ
(ARROW-14744)", {
+
+ dst_dir <- make_temp_dir()
+ dst_file <- file.path(dst_dir, "file.csv")
+ df <- df1[, c("int", "dbl")]
+ write.csv(df, dst_file, row.names = FALSE, quote = FALSE)
+
+ # Mismatch of column names vs schema given via read_options should raise an
error
+ opts <- CsvReadOptions$create(column_names = c("i", "d"))
+ schema <- schema(int = int32(), dbl = float64())
+
+ expect_error(
+ open_dataset(csv_dir, format = "csv", schema = schema, read_options =
opts),
+ "column_names not matching or not found in schema-names"
+ )
Review comment:
Thanks for the test, looking good. One minor suggestion: how about
swapping this for a simpler case, e.g.:
```
open_dataset(csv_dir, format = "csv", schema = schema, column_names =
c("these", "are", "not", "the", "columns", "from", "the", "schema"))
```
It then removes the need to do the more complex work of `opts <-
CsvReadOptions$create(column_names = c("i", "d"))` which is technically
off-label usage that we don't recommend anyway.
##########
File path: r/R/dataset-format.R
##########
@@ -122,6 +122,31 @@ CsvFileFormat$create <- function(...,
opts = csv_file_format_parse_options(...),
convert_options =
csv_file_format_convert_opts(...),
read_options =
csv_file_format_read_opts(...)) {
+
+ options <- list(...)
+ schema <- options[["schema"]]
+
+ column_names <- read_options$column_names
+ schema_names <- names(schema)
+
+ if (!is.null(schema) & !identical(schema_names, column_names)) {
+
+ # Element wise comparison and set differnce of column_names and names in
schema
+ mismatch_colnames <- column_names[match(column_names, schema_names,
nomatch = 0) != seq(column_names)]
+ not_in_schema <- setdiff(schema_names, column_names)
+
+ abort(c(
+ paste(
+ "column_names not matching or not found in schema-names:",
+ deparse1(c(mismatch_colnames, not_in_schema))
+ ),
+ i = "Set column_names to match names of schema",
+ i = "Omit the column_names argument",
+ i = "Omit the read_options argument"
Review comment:
I really love that you've come up with helpful solutions for the
end-user here. However, I have a concern that given that there are multiple
paths that users can take to get here, and `read_options` isn't an argument
that the user is necessarily aware of (it's a few levels down in a function
they wouldn't be likely to call directly), and so this error message might be a
little confusing - it may be sufficient just to state what's wrong, without any
suggestions.
Could I get your thoughts here @jonkeane?
##########
File path: r/R/dataset-format.R
##########
@@ -122,6 +122,31 @@ CsvFileFormat$create <- function(...,
opts = csv_file_format_parse_options(...),
convert_options =
csv_file_format_convert_opts(...),
read_options =
csv_file_format_read_opts(...)) {
+
+ options <- list(...)
+ schema <- options[["schema"]]
+
+ column_names <- read_options$column_names
+ schema_names <- names(schema)
+
+ if (!is.null(schema) & !identical(schema_names, column_names)) {
+
+ # Element wise comparison and set differnce of column_names and names in
schema
+ mismatch_colnames <- column_names[match(column_names, schema_names,
nomatch = 0) != seq(column_names)]
+ not_in_schema <- setdiff(schema_names, column_names)
+
+ abort(c(
+ paste(
+ "column_names not matching or not found in schema-names:",
Review comment:
What do you think to the idea of rephrasing this to use "should" or
"must" as per the tidyverse style guide
(https://style.tidyverse.org/error-messages.html)?
--
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]
