[GitHub] [arrow] thisisnic commented on a change in pull request #12133: ARROW-10485: [R] Accept partitioning in open_dataset when file paths are hive-style

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

thisisnic commented on a change in pull request #12133:
URL: https://github.com/apache/arrow/pull/12133#discussion_r783291608



##########
File path: r/tests/testthat/test-dataset.R
##########
@@ -397,6 +397,59 @@ test_that("Partitioning inference", {
   )
 })
 
+test_that("Specifying partitioning when hive_style", {
+  expected_schema <- open_dataset(hive_dir)$schema
+
+  # If string and names match hive partition names, it's accepted silently
+  ds_with_chr <- open_dataset(hive_dir, partitioning = c("group", "other"))
+  expect_equal(ds_with_chr$schema, expected_schema)
+
+  # If they don't match, we get an error
+  expect_error(
+    open_dataset(hive_dir, partitioning = c("asdf", "zxcv")),
+    '"partitioning" does not match the detected Hive-style partitions: c("group", "other")',
+    fixed = TRUE
+  )
+
+  # If schema and names match, the schema is used to specify the types
+  ds_with_sch <- open_dataset(
+    hive_dir,
+    partitioning = schema(group = int32(), other = utf8())
+  )
+  expect_equal(ds_with_sch$schema, expected_schema)
+
+  ds_with_int8 <- open_dataset(
+    hive_dir,
+    partitioning = schema(group = int8(), other = utf8())
+  )
+  expect_equal(ds_with_int8$schema[["group"]]$type, int8())
+
+  # If they don't match, we get an error
+  expect_error(
+    open_dataset(hive_dir, partitioning = schema(a = int32(), b = utf8())),
+    '"partitioning" does not match the detected Hive-style partitions: c("group", "other")',
+    fixed = TRUE
+  )
+
+

Review comment:
       ```suggestion
   
   ```

##########
File path: r/R/dataset-factory.R
##########
@@ -60,16 +61,65 @@ DatasetFactory$create <- function(x,
     return(FileSystemDatasetFactory$create(path_and_fs$fs, NULL, path_and_fs$path, format))
   }
 
-  if (!is.null(partitioning)) {
-    if (inherits(partitioning, "Schema")) {
-      partitioning <- DirectoryPartitioning$create(partitioning)
-    } else if (is.character(partitioning)) {
-      # These are the column/field names, and we should autodetect their types
-      partitioning <- DirectoryPartitioningFactory$create(partitioning)
+  # Handle partitioning arg in cases where it is "character" or "Schema"
+  if (!is.null(partitioning) && !inherits(partitioning, c("Partitioning", "PartitioningFactory"))) {
+    # isFALSE() would be sufficient but that requires R >= 3.5
+    assert_that(is.logical(hive_style), length(hive_style) == 1)
+    if (hive_style %in% c(TRUE, NA)) {
+      # Default is NA, which means check to see if the paths could be hive_style
+      hive_factory <- HivePartitioningFactory$create()
+      paths <- path_and_fs$fs$ls(
+        path_and_fs$path,
+        allow_not_found = FALSE,
+        recursive = TRUE
+      )
+      hive_schema <- hive_factory$Inspect(paths)
+      # This is length-0 if there are no hive segments
+      if (is.na(hive_style)) {
+        hive_style <- length(hive_schema) > 0
+      }
+    }
+
+    if (hive_style) {
+      if (is.character(partitioning)) {
+        # These are not needed, the user probably provided them because they
+        # thought they needed to. Just make sure they aren't invalid.
+        if (!identical(names(hive_schema), partitioning)) {
+          stop(
+            '"partitioning" does not match the detected Hive-style partitions: ',
+            deparse1(names(hive_schema)),

Review comment:
       Can we rephrase this a little to use "must"/"should" etc language to clarify further to the user what they should do?




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