thisisnic commented on code in PR #13336:
URL: https://github.com/apache/arrow/pull/13336#discussion_r892268059
##########
r/tests/testthat/test-dataset-write.R:
##########
@@ -458,6 +458,8 @@ test_that("Writing a dataset: CSV format options", {
test_that("Dataset writing: unsupported features/input validation", {
skip_if_not_available("parquet")
expect_error(write_dataset(4), "'dataset' must be a Dataset, ")
+ expect_error(write_dataset(data.frame(x = 1, x = 2, check.names = FALSE)),
+ "Duplicated field names")
Review Comment:
Thanks for adding this test, great to have this looked at here explicitly!
I know you didn't write the original error message, but while we're looking
at this, I wonder if we could also look to improve it? The [tidyverse style
guide](https://style.tidyverse.org/error-messages.html) suggests the use of
"must" or "can't", so we could perhaps change it to something like "Field names
must be unique".
##########
r/R/dplyr.R:
##########
@@ -24,6 +24,21 @@ arrow_dplyr_query <- function(.data) {
# RecordBatch, or Dataset) and the state of the user's dplyr query--things
# like selected columns, filters, and group vars.
# An arrow_dplyr_query can contain another arrow_dplyr_query in .data
+
+ supported <- c(
+ "Dataset", "RecordBatch", "RecordBatchReader",
+ "Table", "arrow_dplyr_query", "data.frame"
+ )
+ if (!inherits(.data, supported)) {
+ stop(
+ "'dataset' must be a ",
Review Comment:
The parameter name here is `.data` rather than `'dataset'`. Can you rework
this so that the error message is correct whether it's triggered by
`write_dataset()` or some other bit of code calling `arrow_dplyr_query()`? The
first thing that comes to mind is catching the error again in `write_dataset()`
though that feels a bit inelegant and I don't think it's the best solution.
--
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]
