ARROW-6622: [R] Normalize paths for filesystem API on Windows #5445

nealrichardson · 2019-09-19T21:49:58Z

I fixed this in R, though I wonder if that's not enough. At a minimum, the C++ docs should note this requirement (@pitrou maybe you know the best place in the docs to add this?), and I still think it would be nice if all of this path normalization were handled in C++ (cf. https://issues.apache.org/jira/browse/ARROW-6324).

wesm · 2019-09-19T22:53:31Z

I think I'm with @pitrou here that introducing automatic path normalization into the C++ filesystem API might be a bit too much scope creep -- we could provide a built-in path normalization function for consumers that don't have it (e.g. Python has pathlib, R has its own function here)

nealrichardson · 2019-09-19T23:48:45Z

I can accept that, though we should document the C++ API's requirements accordingly. I created https://issues.apache.org/jira/browse/ARROW-6629 because I didn't see any filesystem docs at all, so nowhere to just add a note in this patch.

nealrichardson · 2019-09-20T03:03:24Z

This is failing, though differently than before: https://ci.appveyor.com/project/nealrichardson/arrow/builds/27536178/job/gu10af0vck7gum81#L4342

The path normalization is making absolute paths with respect to the current working directory in the R process, and that can clash with whatever the FileSystem object has as its path. I'll need to rethink the approach.

codecov-io · 2019-09-21T06:27:12Z

Codecov Report

❗ No coverage uploaded for pull request base (master@2c7fb24). Click here to learn what that means.
The diff coverage is 80%.

@@            Coverage Diff            @@
##             master    #5445   +/-   ##
=========================================
  Coverage          ?   77.18%           
=========================================
  Files             ?       58           
  Lines             ?     4011           
  Branches          ?        0           
=========================================
  Hits              ?     3096           
  Misses            ?      915           
  Partials          ?        0

Impacted Files	Coverage Δ
r/R/io.R	`77.14% <50%> (ø)`
r/R/filesystem.R	`70.45% <82.6%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c7fb24...515d710. Read the comment docs.

nealrichardson · 2019-09-21T22:49:17Z

This is passing and ready for review @romainfrancois

romainfrancois

Should we use fs:: here ?

romainfrancois · 2019-09-23T06:01:35Z

r/R/filesystem.R

  shared_ptr(SubTreeFileSystem, xp)
 }
+
+clean_path <- function(path) {


This looks like things potentially better fixed with fs:: cc @jimhester. I know fs has been downgraded to Suggests here, but I'm just worried we'd be missing cases with the implementation below.

I didn't see the fs function that does the winslash normalization we need here, but more than happy to use it if it exists.

jimhester · 2019-09-23T13:34:09Z

r/R/filesystem.R

+
+fs_relative_path <- function(path) {
+  # Make sure all path separators are "/", not "\" as on Windows
+  path_sep <- ifelse(tolower(Sys.info()[["sysname"]]) == "windows", "\\\\", "/")


You should probably use .Platform$file.sep

Also not sure about the name of this function, it is not really doing anything to compute a relative path, you could probably just use normalizePath(mustWork = FALSE), assuming path doesn't actually have to exist on the filesystem.

Re: .Platform$file.sep, I intended to but then on my Windows VM, .Platform$file.sep reported "/" while tempfile() still created paths with "\" 🤷‍♂

I originally did normalizePath but unfortunately for this use case, normalizePath does path expansion, even with mustWork=FALSE. We need to keep the path relative (to whatever the FileSystem object holds as its cwd) and just need Windows \ to be converted to /. Regular expressions were my last resort and I'm happy to use something better or more robust if it exists.

It looks a bit weird to do a no-op replacement on Unix, though I suppose it doesn't work.

pitrou

This doesn't look correct to me. The simple solution should be to start with /-separated slashes and only use that.

pitrou · 2019-09-23T16:07:53Z

r/R/filesystem.R

-  shared_ptr(Selector, fs___Selector__create(base_dir, allow_non_existent, recursive))
+  shared_ptr(
+    Selector,
+    fs___Selector__create(fs_relative_path(base_dir), allow_non_existent, recursive)


Are you sure? Will it work correctly if base_dir is e.g. a path in an S3 filesystem, or will it produce bogus results?

Same question for all such cases below.

pitrou · 2019-09-23T16:16:32Z

r/tests/testthat/test-filesystem.R

@@ -83,7 +83,6 @@ test_that("SubTreeFilesystem", {
  expect_is(st_fs, "FileSystem")


So you should simply /-normalize td above. Should be easy :-)

At worse you simply string-replace \ with / (but only when on Windows :-)).

That's where I ended up: https://github.com/apache/arrow/pull/5445/files#diff-3bd936ee39b8abe149e5399681610c75R255

Ah, then you shouldn't call it fs_relative_path...

pitrou · 2019-09-23T16:18:29Z

Also note that SubTreeFileSystem is not really needed by end-users. It's mostly useful for internal C++ testing, because there is a sizable generic filesystem test suite that expects to write at the root of an empty filesystem.

nealrichardson · 2019-09-24T22:18:38Z

I renamed the functions to clean_path_abs() and clean_path_rel(). Hopefully that makes their intent and output more clear.

I wouldn't be surprised if we had to revisit this in the future, but for now this seems like a step forward, so I'll merge unless there are objections.

@pitrou

I fixed this in R, though I wonder if that's not enough. At a minimum, the C++ docs should note this requirement (@pitrou maybe you know the best place in the docs to add this?), and I still think it would be nice if all of this path normalization were handled in C++ (cf. https://issues.apache.org/jira/browse/ARROW-6324). Closes apache#5445 from nealrichardson/win-fs-fix and squashes the following commits: 515d710 <Neal Richardson> Rename functions 1775a6d <Neal Richardson> Fix two other absolute paths I missed 0dce0d7 <Neal Richardson> Munge paths directly on windows f03815e <Neal Richardson> Normalize paths for filesystem API on Windows Authored-by: Neal Richardson <[email protected]> Signed-off-by: Neal Richardson <[email protected]>

fsaintjacques added the Component: R label Sep 20, 2019

nealrichardson mentioned this pull request Sep 20, 2019

ARROW-6622 [R]: SubTreeFileSystem path error on Windows #5449

Closed

nealrichardson force-pushed the win-fs-fix branch from bef7f7f to a1c1e3f Compare September 20, 2019 21:34

nealrichardson requested a review from romainfrancois September 21, 2019 22:49

romainfrancois reviewed Sep 23, 2019

View reviewed changes

jimhester reviewed Sep 23, 2019

View reviewed changes

pitrou reviewed Sep 23, 2019

View reviewed changes

nealrichardson added 4 commits September 24, 2019 13:50

Normalize paths for filesystem API on Windows

f03815e

Munge paths directly on windows

0dce0d7

Fix two other absolute paths I missed

1775a6d

Rename functions

515d710

nealrichardson force-pushed the win-fs-fix branch from 8de4cbf to 515d710 Compare September 24, 2019 22:15

nealrichardson closed this in 0d0e4cc Sep 25, 2019

nealrichardson deleted the win-fs-fix branch September 25, 2019 14:05

asfimport mentioned this pull request Sep 26, 2019

[C++][R] SubTreeFileSystem path error on Windows #22976

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-6622: [R] Normalize paths for filesystem API on Windows #5445

ARROW-6622: [R] Normalize paths for filesystem API on Windows #5445

nealrichardson commented Sep 19, 2019

wesm commented Sep 19, 2019

nealrichardson commented Sep 19, 2019

nealrichardson commented Sep 20, 2019

codecov-io commented Sep 21, 2019 •

edited

Loading

nealrichardson commented Sep 21, 2019

romainfrancois left a comment

romainfrancois Sep 23, 2019

nealrichardson Sep 23, 2019

jimhester Sep 23, 2019

jimhester Sep 23, 2019

nealrichardson Sep 23, 2019 •

edited

Loading

pitrou Sep 23, 2019

pitrou left a comment

pitrou Sep 23, 2019

pitrou Sep 23, 2019

pitrou Sep 23, 2019

nealrichardson Sep 23, 2019

pitrou Sep 23, 2019

nealrichardson Sep 23, 2019

pitrou Sep 23, 2019

pitrou commented Sep 23, 2019

nealrichardson commented Sep 24, 2019

		@@ -83,7 +83,6 @@ test_that("SubTreeFilesystem", {
		expect_is(st_fs, "FileSystem")

ARROW-6622: [R] Normalize paths for filesystem API on Windows #5445

ARROW-6622: [R] Normalize paths for filesystem API on Windows #5445

Conversation

nealrichardson commented Sep 19, 2019

wesm commented Sep 19, 2019

nealrichardson commented Sep 19, 2019

nealrichardson commented Sep 20, 2019

codecov-io commented Sep 21, 2019 • edited Loading

Codecov Report

nealrichardson commented Sep 21, 2019

romainfrancois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nealrichardson Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Sep 23, 2019

nealrichardson commented Sep 24, 2019

codecov-io commented Sep 21, 2019 •

edited

Loading

nealrichardson Sep 23, 2019 •

edited

Loading