From f03815e078f40f086d175b33ae3671b77be75da5 Mon Sep 17 00:00:00 2001 From: Neal Richardson Date: Thu, 19 Sep 2019 14:44:44 -0700 Subject: [PATCH 1/4] Normalize paths for filesystem API on Windows --- r/R/filesystem.R | 48 ++++++++++++++++++++---------- r/R/io.R | 11 ++++--- r/tests/testthat/test-filesystem.R | 1 - 3 files changed, 37 insertions(+), 23 deletions(-) diff --git a/r/R/filesystem.R b/r/R/filesystem.R index d20edcc6c024c..7357ca35a1396 100644 --- a/r/R/filesystem.R +++ b/r/R/filesystem.R @@ -108,7 +108,10 @@ Selector <- R6Class("Selector", ) Selector$create <- function(base_dir, allow_non_existent = FALSE, recursive = FALSE) { - shared_ptr(Selector, fs___Selector__create(base_dir, allow_non_existent, recursive)) + shared_ptr( + Selector, + fs___Selector__create(clean_path(base_dir), allow_non_existent, recursive) + ) } #' @title FileSystem classes @@ -165,53 +168,61 @@ FileSystem <- R6Class("FileSystem", inherit = Object, public = list( GetTargetStats = function(x) { if (inherits(x, "Selector")) { - map(fs___FileSystem__GetTargetStats_Selector(self, x), shared_ptr, class = FileStats) + map( + fs___FileSystem__GetTargetStats_Selector(self, x), + shared_ptr, + class = FileStats + ) } else if (is.character(x)){ - map(fs___FileSystem__GetTargetStats_Paths(self, x), shared_ptr, class = FileStats) + map( + fs___FileSystem__GetTargetStats_Paths(self, clean_path(x)), + shared_ptr, + class = FileStats + ) } else { - abort("incompatible type for FileSystem$GetTargetStarts()") + abort("incompatible type for FileSystem$GetTargetStats()") } }, CreateDir = function(path, recursive = TRUE) { - fs___FileSystem__CreateDir(self, path, isTRUE(recursive)) + fs___FileSystem__CreateDir(self, clean_path(path), isTRUE(recursive)) }, DeleteDir = function(path) { - fs___FileSystem__DeleteDir(self, path) + fs___FileSystem__DeleteDir(self, clean_path(path)) }, DeleteDirContents = function(path) { - fs___FileSystem__DeleteDirContents(self, path) + fs___FileSystem__DeleteDirContents(self, clean_path(path)) }, DeleteFile = function(path) { - fs___FileSystem__DeleteFile(self, path) + fs___FileSystem__DeleteFile(self, clean_path(path)) }, DeleteFiles = function(paths) { - fs___FileSystem__DeleteFiles(self, paths) + fs___FileSystem__DeleteFiles(self, clean_path(paths)) }, Move = function(src, dest) { - fs___FileSystem__Move(self, src, dest) + fs___FileSystem__Move(self, clean_path(src), clean_path(dest)) }, CopyFile = function(src, dest) { - fs___FileSystem__CopyFile(self, src, dest) + fs___FileSystem__CopyFile(self, clean_path(src), clean_path(dest)) }, OpenInputStream = function(path) { - shared_ptr(InputStream, fs___FileSystem__OpenInputStream(self, path)) + shared_ptr(InputStream, fs___FileSystem__OpenInputStream(self, clean_path(path))) }, OpenInputFile = function(path) { - shared_ptr(InputStream, fs___FileSystem__OpenInputFile(self, path)) + shared_ptr(InputStream, fs___FileSystem__OpenInputFile(self, clean_path(path))) }, OpenOutputStream = function(path) { - shared_ptr(OutputStream, fs___FileSystem__OpenOutputStream(self, path)) + shared_ptr(OutputStream, fs___FileSystem__OpenOutputStream(self, clean_path(path))) }, OpenAppendStream = function(path) { - shared_ptr(OutputStream, fs___FileSystem__OpenAppendStream(self, path)) + shared_ptr(OutputStream, fs___FileSystem__OpenAppendStream(self, clean_path(path))) } ) ) @@ -232,6 +243,11 @@ LocalFileSystem$create <- function() { #' @export SubTreeFileSystem <- R6Class("SubTreeFileSystem", inherit = FileSystem) SubTreeFileSystem$create <- function(base_path, base_fs) { - xp <- fs___SubTreeFileSystem__create(base_path, base_fs) + xp <- fs___SubTreeFileSystem__create(clean_path(base_path), base_fs) shared_ptr(SubTreeFileSystem, xp) } + +clean_path <- function(path) { + # Make sure we have a valid, forward-slashed path for passing to Arrow + normalizePath(path, winslash = "/", mustWork = FALSE) +} diff --git a/r/R/io.R b/r/R/io.R index f5390e32b257b..bafce7a3e0c36 100644 --- a/r/R/io.R +++ b/r/R/io.R @@ -74,8 +74,7 @@ OutputStream <- R6Class("OutputStream", inherit = Writable, #' @export FileOutputStream <- R6Class("FileOutputStream", inherit = OutputStream) FileOutputStream$create <- function(path) { - path <- normalizePath(path, mustWork = FALSE) - shared_ptr(FileOutputStream, io___FileOutputStream__Open(path)) + shared_ptr(FileOutputStream, io___FileOutputStream__Open(clean_path(path))) } #' @usage NULL @@ -148,7 +147,7 @@ Readable <- R6Class("Readable", inherit = Object, #' #' @section Methods: #' -#' - `$GetSize()`: +#' - `$GetSize()`: #' - `$supports_zero_copy()`: Logical #' - `$seek(position)`: go to that position in the stream #' - `$tell()`: return the position in the stream @@ -210,7 +209,7 @@ MemoryMappedFile <- R6Class("MemoryMappedFile", inherit = RandomAccessFile, #' @export ReadableFile <- R6Class("ReadableFile", inherit = RandomAccessFile) ReadableFile$create <- function(path) { - shared_ptr(ReadableFile, io___ReadableFile__Open(normalizePath(path))) + shared_ptr(ReadableFile, io___ReadableFile__Open(clean_path(path))) } #' @usage NULL @@ -232,7 +231,7 @@ BufferReader$create <- function(x) { #' #' @export mmap_create <- function(path, size) { - path <- normalizePath(path, mustWork = FALSE) + path <- clean_path(path) shared_ptr(MemoryMappedFile, io___MemoryMappedFile__Create(path, size)) } @@ -244,7 +243,7 @@ mmap_create <- function(path, size) { #' @export mmap_open <- function(path, mode = c("read", "write", "readwrite")) { mode <- match(match.arg(mode), c("read", "write", "readwrite")) - 1L - path <- normalizePath(path) + path <- clean_path(path) shared_ptr(MemoryMappedFile, io___MemoryMappedFile__Open(path, mode)) } diff --git a/r/tests/testthat/test-filesystem.R b/r/tests/testthat/test-filesystem.R index 1c19838306640..41327c077cf58 100644 --- a/r/tests/testthat/test-filesystem.R +++ b/r/tests/testthat/test-filesystem.R @@ -83,7 +83,6 @@ test_that("SubTreeFilesystem", { expect_is(st_fs, "FileSystem") st_fs$CreateDir("test") st_fs$CopyFile("DESCRIPTION", "DESC.txt") - skip_on_os("windows") # See ARROW-6622 stats <- st_fs$GetTargetStats(c("DESCRIPTION", "test", "nope", "DESC.txt")) expect_equal(stats[[1L]]$type, FileType$File) expect_equal(stats[[2L]]$type, FileType$Directory) From 0dce0d7808e2657116a4e33d36709170c8ff4eea Mon Sep 17 00:00:00 2001 From: Neal Richardson Date: Fri, 20 Sep 2019 14:34:43 -0700 Subject: [PATCH 2/4] Munge paths directly on windows --- r/R/filesystem.R | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/r/R/filesystem.R b/r/R/filesystem.R index 7357ca35a1396..9ca7c48690e06 100644 --- a/r/R/filesystem.R +++ b/r/R/filesystem.R @@ -110,7 +110,7 @@ Selector <- R6Class("Selector", Selector$create <- function(base_dir, allow_non_existent = FALSE, recursive = FALSE) { shared_ptr( Selector, - fs___Selector__create(clean_path(base_dir), allow_non_existent, recursive) + fs___Selector__create(fs_relative_path(base_dir), allow_non_existent, recursive) ) } @@ -175,7 +175,7 @@ FileSystem <- R6Class("FileSystem", inherit = Object, ) } else if (is.character(x)){ map( - fs___FileSystem__GetTargetStats_Paths(self, clean_path(x)), + fs___FileSystem__GetTargetStats_Paths(self, fs_relative_path(x)), shared_ptr, class = FileStats ) @@ -185,44 +185,44 @@ FileSystem <- R6Class("FileSystem", inherit = Object, }, CreateDir = function(path, recursive = TRUE) { - fs___FileSystem__CreateDir(self, clean_path(path), isTRUE(recursive)) + fs___FileSystem__CreateDir(self, fs_relative_path(path), isTRUE(recursive)) }, DeleteDir = function(path) { - fs___FileSystem__DeleteDir(self, clean_path(path)) + fs___FileSystem__DeleteDir(self, fs_relative_path(path)) }, DeleteDirContents = function(path) { - fs___FileSystem__DeleteDirContents(self, clean_path(path)) + fs___FileSystem__DeleteDirContents(self, fs_relative_path(path)) }, DeleteFile = function(path) { - fs___FileSystem__DeleteFile(self, clean_path(path)) + fs___FileSystem__DeleteFile(self, fs_relative_path(path)) }, DeleteFiles = function(paths) { - fs___FileSystem__DeleteFiles(self, clean_path(paths)) + fs___FileSystem__DeleteFiles(self, fs_relative_path(paths)) }, Move = function(src, dest) { - fs___FileSystem__Move(self, clean_path(src), clean_path(dest)) + fs___FileSystem__Move(self, clean_path(src), fs_relative_path(dest)) }, CopyFile = function(src, dest) { - fs___FileSystem__CopyFile(self, clean_path(src), clean_path(dest)) + fs___FileSystem__CopyFile(self, clean_path(src), fs_relative_path(dest)) }, OpenInputStream = function(path) { - shared_ptr(InputStream, fs___FileSystem__OpenInputStream(self, clean_path(path))) + shared_ptr(InputStream, fs___FileSystem__OpenInputStream(self, fs_relative_path(path))) }, OpenInputFile = function(path) { - shared_ptr(InputStream, fs___FileSystem__OpenInputFile(self, clean_path(path))) + shared_ptr(InputStream, fs___FileSystem__OpenInputFile(self, fs_relative_path(path))) }, OpenOutputStream = function(path) { - shared_ptr(OutputStream, fs___FileSystem__OpenOutputStream(self, clean_path(path))) + shared_ptr(OutputStream, fs___FileSystem__OpenOutputStream(self, fs_relative_path(path))) }, OpenAppendStream = function(path) { - shared_ptr(OutputStream, fs___FileSystem__OpenAppendStream(self, clean_path(path))) + shared_ptr(OutputStream, fs___FileSystem__OpenAppendStream(self, fs_relative_path(path))) } ) ) @@ -243,11 +243,17 @@ LocalFileSystem$create <- function() { #' @export SubTreeFileSystem <- R6Class("SubTreeFileSystem", inherit = FileSystem) SubTreeFileSystem$create <- function(base_path, base_fs) { - xp <- fs___SubTreeFileSystem__create(clean_path(base_path), base_fs) + xp <- fs___SubTreeFileSystem__create(fs_relative_path(base_path), base_fs) shared_ptr(SubTreeFileSystem, xp) } clean_path <- function(path) { - # Make sure we have a valid, forward-slashed path for passing to Arrow + # Make sure we have a valid, absolute, forward-slashed path for passing to Arrow normalizePath(path, winslash = "/", mustWork = FALSE) } + +fs_relative_path <- function(path) { + # Make sure all path separators are "/", not "\" as on Windows + path_sep <- ifelse(tolower(Sys.info()[["sysname"]]) == "windows", "\\\\", "/") + gsub(path_sep, "/", path) +} From 1775a6d38f60a7d49d6a29465c1e0aeca05ec7a8 Mon Sep 17 00:00:00 2001 From: Neal Richardson Date: Fri, 20 Sep 2019 16:52:05 -0700 Subject: [PATCH 3/4] Fix two other absolute paths I missed --- r/R/filesystem.R | 4 ++-- r/tests/testthat/test-filesystem.R | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/r/R/filesystem.R b/r/R/filesystem.R index 9ca7c48690e06..e71ea951102a3 100644 --- a/r/R/filesystem.R +++ b/r/R/filesystem.R @@ -205,11 +205,11 @@ FileSystem <- R6Class("FileSystem", inherit = Object, }, Move = function(src, dest) { - fs___FileSystem__Move(self, clean_path(src), fs_relative_path(dest)) + fs___FileSystem__Move(self, fs_relative_path(src), fs_relative_path(dest)) }, CopyFile = function(src, dest) { - fs___FileSystem__CopyFile(self, clean_path(src), fs_relative_path(dest)) + fs___FileSystem__CopyFile(self, fs_relative_path(src), fs_relative_path(dest)) }, OpenInputStream = function(path) { diff --git a/r/tests/testthat/test-filesystem.R b/r/tests/testthat/test-filesystem.R index 41327c077cf58..c0565adcfc09b 100644 --- a/r/tests/testthat/test-filesystem.R +++ b/r/tests/testthat/test-filesystem.R @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. -context("test-type") +context("File system") test_that("LocalFilesystem", { fs <- LocalFileSystem$create() From 515d7105bf12b45997725fe2134b62c0f325437b Mon Sep 17 00:00:00 2001 From: Neal Richardson Date: Tue, 24 Sep 2019 15:15:41 -0700 Subject: [PATCH 4/4] Rename functions --- r/R/filesystem.R | 32 ++++++++++++++++---------------- r/R/io.R | 8 ++++---- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/r/R/filesystem.R b/r/R/filesystem.R index e71ea951102a3..ce507cc3e50a4 100644 --- a/r/R/filesystem.R +++ b/r/R/filesystem.R @@ -110,7 +110,7 @@ Selector <- R6Class("Selector", Selector$create <- function(base_dir, allow_non_existent = FALSE, recursive = FALSE) { shared_ptr( Selector, - fs___Selector__create(fs_relative_path(base_dir), allow_non_existent, recursive) + fs___Selector__create(clean_path_rel(base_dir), allow_non_existent, recursive) ) } @@ -175,7 +175,7 @@ FileSystem <- R6Class("FileSystem", inherit = Object, ) } else if (is.character(x)){ map( - fs___FileSystem__GetTargetStats_Paths(self, fs_relative_path(x)), + fs___FileSystem__GetTargetStats_Paths(self, clean_path_rel(x)), shared_ptr, class = FileStats ) @@ -185,44 +185,44 @@ FileSystem <- R6Class("FileSystem", inherit = Object, }, CreateDir = function(path, recursive = TRUE) { - fs___FileSystem__CreateDir(self, fs_relative_path(path), isTRUE(recursive)) + fs___FileSystem__CreateDir(self, clean_path_rel(path), isTRUE(recursive)) }, DeleteDir = function(path) { - fs___FileSystem__DeleteDir(self, fs_relative_path(path)) + fs___FileSystem__DeleteDir(self, clean_path_rel(path)) }, DeleteDirContents = function(path) { - fs___FileSystem__DeleteDirContents(self, fs_relative_path(path)) + fs___FileSystem__DeleteDirContents(self, clean_path_rel(path)) }, DeleteFile = function(path) { - fs___FileSystem__DeleteFile(self, fs_relative_path(path)) + fs___FileSystem__DeleteFile(self, clean_path_rel(path)) }, DeleteFiles = function(paths) { - fs___FileSystem__DeleteFiles(self, fs_relative_path(paths)) + fs___FileSystem__DeleteFiles(self, clean_path_rel(paths)) }, Move = function(src, dest) { - fs___FileSystem__Move(self, fs_relative_path(src), fs_relative_path(dest)) + fs___FileSystem__Move(self, clean_path_rel(src), clean_path_rel(dest)) }, CopyFile = function(src, dest) { - fs___FileSystem__CopyFile(self, fs_relative_path(src), fs_relative_path(dest)) + fs___FileSystem__CopyFile(self, clean_path_rel(src), clean_path_rel(dest)) }, OpenInputStream = function(path) { - shared_ptr(InputStream, fs___FileSystem__OpenInputStream(self, fs_relative_path(path))) + shared_ptr(InputStream, fs___FileSystem__OpenInputStream(self, clean_path_rel(path))) }, OpenInputFile = function(path) { - shared_ptr(InputStream, fs___FileSystem__OpenInputFile(self, fs_relative_path(path))) + shared_ptr(InputStream, fs___FileSystem__OpenInputFile(self, clean_path_rel(path))) }, OpenOutputStream = function(path) { - shared_ptr(OutputStream, fs___FileSystem__OpenOutputStream(self, fs_relative_path(path))) + shared_ptr(OutputStream, fs___FileSystem__OpenOutputStream(self, clean_path_rel(path))) }, OpenAppendStream = function(path) { - shared_ptr(OutputStream, fs___FileSystem__OpenAppendStream(self, fs_relative_path(path))) + shared_ptr(OutputStream, fs___FileSystem__OpenAppendStream(self, clean_path_rel(path))) } ) ) @@ -243,16 +243,16 @@ LocalFileSystem$create <- function() { #' @export SubTreeFileSystem <- R6Class("SubTreeFileSystem", inherit = FileSystem) SubTreeFileSystem$create <- function(base_path, base_fs) { - xp <- fs___SubTreeFileSystem__create(fs_relative_path(base_path), base_fs) + xp <- fs___SubTreeFileSystem__create(clean_path_rel(base_path), base_fs) shared_ptr(SubTreeFileSystem, xp) } -clean_path <- function(path) { +clean_path_abs <- function(path) { # Make sure we have a valid, absolute, forward-slashed path for passing to Arrow normalizePath(path, winslash = "/", mustWork = FALSE) } -fs_relative_path <- function(path) { +clean_path_rel <- function(path) { # Make sure all path separators are "/", not "\" as on Windows path_sep <- ifelse(tolower(Sys.info()[["sysname"]]) == "windows", "\\\\", "/") gsub(path_sep, "/", path) diff --git a/r/R/io.R b/r/R/io.R index bafce7a3e0c36..255dc1e82412f 100644 --- a/r/R/io.R +++ b/r/R/io.R @@ -74,7 +74,7 @@ OutputStream <- R6Class("OutputStream", inherit = Writable, #' @export FileOutputStream <- R6Class("FileOutputStream", inherit = OutputStream) FileOutputStream$create <- function(path) { - shared_ptr(FileOutputStream, io___FileOutputStream__Open(clean_path(path))) + shared_ptr(FileOutputStream, io___FileOutputStream__Open(clean_path_abs(path))) } #' @usage NULL @@ -209,7 +209,7 @@ MemoryMappedFile <- R6Class("MemoryMappedFile", inherit = RandomAccessFile, #' @export ReadableFile <- R6Class("ReadableFile", inherit = RandomAccessFile) ReadableFile$create <- function(path) { - shared_ptr(ReadableFile, io___ReadableFile__Open(clean_path(path))) + shared_ptr(ReadableFile, io___ReadableFile__Open(clean_path_abs(path))) } #' @usage NULL @@ -231,7 +231,7 @@ BufferReader$create <- function(x) { #' #' @export mmap_create <- function(path, size) { - path <- clean_path(path) + path <- clean_path_abs(path) shared_ptr(MemoryMappedFile, io___MemoryMappedFile__Create(path, size)) } @@ -243,7 +243,7 @@ mmap_create <- function(path, size) { #' @export mmap_open <- function(path, mode = c("read", "write", "readwrite")) { mode <- match(match.arg(mode), c("read", "write", "readwrite")) - 1L - path <- clean_path(path) + path <- clean_path_abs(path) shared_ptr(MemoryMappedFile, io___MemoryMappedFile__Open(path, mode)) }