GH-38084: [R] Do not memory map when explicitly checking for file rem…

…oval (#38085)

### Rationale for this change

We have failing CI on Windows because removing files that have memory mapped sections is not allowed.

### What changes are included in this PR?

Pass `mmap = FALSE` when we explicitly check for removal. I think the other cases are OK because `unlink()` fails silently (which is maybe why we haven't seen mass CI failures because of this issue before).

### Are these changes tested?

The changes are covered by existing tests.

### Are there any user-facing changes?

No.

* Closes: #38084

Authored-by: Dewey Dunnington <[email protected]>
Signed-off-by: Dewey Dunnington <[email protected]>

r/R/parquet.R

@@ -22,6 +22,7 @@
 #'
 #' @inheritParams read_feather
 #' @param props [ParquetArrowReaderProperties]
+#' @param mmap Use TRUE to use memory mapping where possible
 #' @param ... Additional arguments passed to `ParquetFileReader$create()`
 #'
 #' @return A `tibble` if `as_data_frame` is `TRUE` (the default), or an
@@ -43,14 +44,15 @@ read_parquet <- function(file,
                          # Assembling `props` yourself is something you do with
                          # ParquetFileReader but not here.
                          props = ParquetArrowReaderProperties$create(),
+                         mmap = TRUE,
                          ...) {
   if (!inherits(file, "RandomAccessFile")) {
     # Compression is handled inside the parquet file format, so we don't need
     # to detect from the file extension and wrap in a CompressedInputStream
-    file <- make_readable_file(file)
+    file <- make_readable_file(file, mmap = mmap)
     on.exit(file$close())
   }
-  reader <- ParquetFileReader$create(file, props = props, ...)
+  reader <- ParquetFileReader$create(file, props = props, mmap = mmap, ...)
 
   col_select <- enquo(col_select)
   if (!quo_is_null(col_select)) {

r/tests/testthat/test-parquet.R

@@ -32,7 +32,7 @@ test_that("simple int column roundtrip", {
   pq_tmp_file <- tempfile() # You can specify the .parquet here but that's probably not necessary
 
   write_parquet(df, pq_tmp_file)
-  df_read <- read_parquet(pq_tmp_file)
+  df_read <- read_parquet(pq_tmp_file, mmap = FALSE)
   expect_equal(df, df_read)
   # Make sure file connection is cleaned up
   expect_error(file.remove(pq_tmp_file), NA)
@@ -288,15 +288,21 @@ test_that("write_parquet() returns its input", {
 
 test_that("write_parquet() handles version argument", {
   df <- tibble::tibble(x = 1:5)
-  tf <- tempfile()
-  on.exit(unlink(tf))
 
-  purrr::walk(list("1.0", "2.4", "2.6", "latest", 1.0, 2.4, 2.6, 1L), ~ {
-    write_parquet(df, tf, version = .x)
+  versions <- list("1.0", "2.4", "2.6", "latest", 1.0, 2.4, 2.6, 1L)
+  purrr::walk(versions, function(x) {
+    tf <- tempfile()
+    on.exit(unlink(tf))
+
+    write_parquet(df, tf, version = x)
     expect_identical(read_parquet(tf), df)
   })
-  purrr::walk(list("3.0", 3.0, 3L, "A"), ~ {
-    expect_error(write_parquet(df, tf, version = .x))
+
+  invalid_versions <- list("3.0", 3.0, 3L, "A")
+  purrr::walk(invalid_versions, function(x) {
+    tf <- tempfile()
+    on.exit(unlink(tf))
+    expect_error(write_parquet(df, tf, version = x))
   })
 })
 

r/tests/testthat/test-read-record-batch.R

@@ -38,7 +38,7 @@ test_that("RecordBatchFileWriter / RecordBatchFileReader roundtrips", {
   writer$close()
   stream$close()
 
-  expect_equal(read_feather(tf, as_data_frame = FALSE), tab)
+  expect_equal(read_feather(tf, as_data_frame = FALSE, mmap = FALSE), tab)
   # Make sure connections are closed
   expect_error(file.remove(tf), NA)
   skip_on_os("windows") # This should pass, we've closed the stream