[GitHub] [arrow] nealrichardson commented on a change in pull request #9566: ARROW-11756: [R] passing a partition as a schema leads to segfaults

[GitBox <gi...@apache.org>]

nealrichardson commented on a change in pull request #9566:
URL: https://github.com/apache/arrow/pull/9566#discussion_r582406620



##########
File path: r/tests/testthat/test-dataset.R
##########
@@ -194,6 +194,38 @@ test_that("Hive partitioning", {
   )
 })
 
+test_that("open_dataset fails when partitioning is passed as schema", {

Review comment:
       I'd call this something about "input validation"

##########
File path: r/R/dataset.R
##########
@@ -64,6 +64,9 @@ open_dataset <- function(sources,
                          partitioning = hive_partition(),
                          unify_schemas = NULL,
                          ...) {
+  if (isFALSE(inherits(schema, "Schema")) && isFALSE(is.null(schema))) {
+    stop("'schema' must be a Schema object", call. = FALSE)
+  }

Review comment:
       I think the better place for this check is lower down, in DatasetFactory$Finish. Add `assert_is(schema, "Schema")` before L30 in dataset-factory.R
   
   The reasoning is that since we're trying to prevent a segfault in the C++ code, we should put the validation as close to the place where we get into the C++ code. That way, if someone were to be using the lower-level interface to the C++ library, they're still protected from these cases. 

##########
File path: r/tests/testthat/test-dataset.R
##########
@@ -194,6 +194,38 @@ test_that("Hive partitioning", {
   )
 })
 
+test_that("open_dataset fails when partitioning is passed as schema", {
+  expect_error(
+    open_dataset(hive_dir, hive_partition(other = utf8(), group = uint8()))
+  )
+
+  # skipping schema
+  ds <- open_dataset(hive_dir, , hive_partition(other = utf8(), group = uint8()))

Review comment:
       I don't think these next test cases are necessary, you're just testing how R matches function arguments, and they would test without your change.

##########
File path: r/R/dataset.R
##########
@@ -64,6 +64,9 @@ open_dataset <- function(sources,
                          partitioning = hive_partition(),
                          unify_schemas = NULL,
                          ...) {
+  if (isFALSE(inherits(schema, "Schema")) && isFALSE(is.null(schema))) {
+    stop("'schema' must be a Schema object", call. = FALSE)
+  }

Review comment:
       By the same reasoning, we should `assert_is(schema, "Schema")` on L80 as well because there is a different C++ entry point on L85.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org