Skip to content

Conversation

@romainfrancois
Copy link
Contributor

No description provided.

@romainfrancois
Copy link
Contributor Author

@romainfrancois romainfrancois marked this pull request as ready for review September 18, 2019 12:53
@romainfrancois
Copy link
Contributor Author

@nealrichardson I've added some basic tests. I guess we'd do s3 file system in another PR ?

@nealrichardson
Copy link
Member

Yes, we'll do S3 on https://issues.apache.org/jira/browse/ARROW-6439.

@nealrichardson
Copy link
Member

Appveyor failed: https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/27494817/job/t9yhu00mpe0pcfkp#L4093

> test_check("arrow")
  -- 1. Error: SubTreeFilesystem (@test-filesystem.R#86)  ------------------------
  Unknown error: Underlying filesystem returned path 'C:/Users/appveyor/AppData/Local/Temp/1/RtmpwbPLFA/working_dir/Rtmpu4pBCJ/file9a84a3f668f/DESCRIPTION', which is not a subpath of 'C:/Users/appveyor/AppData/Local/Temp/1\RtmpwbPLFA/working_dir\Rtmpu4pBCJ\file9a84a3f668f/'
  1: st_fs$GetTargetStats(c("DESCRIPTION", "test", "nope", "DESC.txt")) at testthat/test-filesystem.R:86
  2: map(fs___FileSystem__GetTargetStats_Paths(self, x), shared_ptr, class = FileStats)
  3: fs___FileSystem__GetTargetStats_Paths(self, x)
  
  == testthat results  ===========================================================
  [ OK: 992 | SKIPPED: 2 | WARNINGS: 0 | FAILED: 1 ]

Copy link
Member

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems straightforward enough for now. I didn't look closely at the cpp code but it doesn't look controversial. Just a couple of notes, plus the Windows build failure.

@nealrichardson
Copy link
Member

Looks good except for this documentation warning: https://travis-ci.org/apache/arrow/jobs/586900859#L3378

@romainfrancois romainfrancois deleted the ARROW-6438/FileSystem branch September 20, 2019 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants