ARROW-6439: [R] Implement S3 file-system interface in R #6901

nealrichardson · 2020-04-11T21:13:12Z

This patch does the foundational work to conditionally build the S3 bindings if the C++ library was built with ARROW_S3=ON. It also adds bindings for FileSystemFromUri and enough wiring up in the datasets code so that open_dataset("s3://ursa-labs-taxi-data", partitioning = c("year", "month")) works.

There's lots of other S3FileSystem methods that probably should be implemented and aren't here. Also, calling S3FileSystem$create() segfaults. But FileSystemFromUri works fine.

github-actions · 2020-04-11T21:16:31Z

https://issues.apache.org/jira/browse/ARROW-6439

fsaintjacques

I fixed the segfault.

> ds <- open_dataset("file:///home/fsaintjacques/datasets/nyc-tlc/parquet", format="parquet")
> ds %>% filter(total_amount > 1000) %>% select(passenger_count) %>% collect()
# A tibble: 746 x 1
   passenger_count
             <int>
 1               1
 2               1
 3               1
 4               1
 5               1
 6               1
 7               1
 8               1
 9               4
10               1
# … with 736 more rows
> ds_s3 <- open_dataset("s3://123:12345678@nyc-tlc/parquet?scheme=http&endpoint_override=localhost:9000", format="parquet")
> ds_s3 %>% filter(total_amount > 1000) %>% select(passenger_count) %>% collect()
# A tibble: 746 x 1
   passenger_count
             <int>
 1               1
 2               1
 3               1
 4               1
 5               1
 6               1
 7               1
 8               1
 9               1
10               4
# … with 736 more rows

fsaintjacques · 2020-04-16T18:22:31Z

r/R/filesystem.R

+#' @export
+S3FileSystem <- R6Class("S3FileSystem", inherit = FileSystem)
+S3FileSystem$create <- function() {
+  fs___EnsureS3Initialized()


Should you just hide this in fs___S3FileSystem__create instead?

Could, sure. Wasn't sure how many functions I would need to write and whether they'd all need it.

fsaintjacques · 2020-04-16T18:34:12Z

r/src/filesystem.cpp

+// [[arrow::export]]
+Rcpp::List fs___FileSystemFromUri(const std::string& path) {
+  std::string out_path;
+  auto file_system = VALUE_OR_STOP(fs::FileSystemFromUri(path, &out_path));


Created ticket https://issues.apache.org/jira/browse/ARROW-8488, prefer ValueOrStop instead.

fsaintjacques · 2020-04-16T19:01:17Z

r/tests/testthat/test-filesystem.R

+test_that("FileSystem$from_uri", {
+  skip_on_cran()
+  skip_if_not_available("s3")
+  fs_and_path <- FileSystem$from_uri("s3://ursa-labs-taxi-data")


I would just make sure this works with a non-existing bucket so we don't depend on the presence of ursa-labs-taxi-data bucket.

Does it work with a bucket that doesn't exist? I actually didn't try.

We already talk about the ursa-labs-taxi-data bucket in r/vignettes/dataset.Rmd, so the presence of the bucket is already "depended on".

fsaintjacques · 2020-04-16T19:03:18Z

r/data-raw/codegen.R

-  pmap_chr(function(name, return_type, args, file, line){
+
+all_decorations <- cpp_decorations()
+arrow_exports <- get_exported_functions(all_decorations, c("arrow", "s3"))


WIll you want to do this for other component, e.g. json, csv, parquet, dataset...

With this change we could if we wanted to enable building slimmer versions of the package, but I'm not planning on doing that unless there's a compelling reason to.

fsaintjacques · 2020-04-16T20:13:02Z

Will wait for CI, then merge.

nealrichardson

Thanks @fsaintjacques

ci/scripts/r_test.sh

nealrichardson · 2020-04-16T20:34:47Z

r/R/filesystem.R

+#' @export
+S3FileSystem <- R6Class("S3FileSystem", inherit = FileSystem)
+S3FileSystem$create <- function() {
+  fs___EnsureS3Initialized()


Could, sure. Wasn't sure how many functions I would need to write and whether they'd all need it.

nealrichardson · 2020-04-16T20:37:07Z

r/data-raw/codegen.R

-  pmap_chr(function(name, return_type, args, file, line){
+
+all_decorations <- cpp_decorations()
+arrow_exports <- get_exported_functions(all_decorations, c("arrow", "s3"))


With this change we could if we wanted to enable building slimmer versions of the package, but I'm not planning on doing that unless there's a compelling reason to.

nealrichardson · 2020-04-16T20:39:19Z

r/tests/testthat/test-filesystem.R

+test_that("FileSystem$from_uri", {
+  skip_on_cran()
+  skip_if_not_available("s3")
+  fs_and_path <- FileSystem$from_uri("s3://ursa-labs-taxi-data")


Does it work with a bucket that doesn't exist? I actually didn't try.

We already talk about the ursa-labs-taxi-data bucket in r/vignettes/dataset.Rmd, so the presence of the bucket is already "depended on".

nealrichardson

I added the comments, will accept the suggestions and merge

ci/scripts/cpp_test.sh

ci/scripts/r_test.sh

This patch does the foundational work to conditionally build the S3 bindings if the C++ library was built with ARROW_S3=ON. It also adds bindings for `FileSystemFromUri` and enough wiring up in the datasets code so that `open_dataset("s3://ursa-labs-taxi-data", partitioning = c("year", "month"))` works. There's lots of other S3FileSystem methods that probably should be implemented and aren't here. Also, calling `S3FileSystem$create()` segfaults. But `FileSystemFromUri` works fine. Closes #6901 from nealrichardson/r-s3 Lead-authored-by: Neal Richardson <neal.p.richardson@gmail.com> Co-authored-by: François Saint-Jacques <fsaintjacques@gmail.com> Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>

nealrichardson marked this pull request as ready for review April 13, 2020 19:51

nealrichardson force-pushed the r-s3 branch from a428beb to 2fc8836 Compare April 14, 2020 15:59

nealrichardson added 5 commits April 15, 2020 13:27

Set up machinery for building s3 bindings if ARROW_S3 is on

dd7e72e

Add FileSystem$from_uri

8667d12

Wire up FileSystem$from_uri in DatasetFactory$create

cce3bc5

Lint

124b502

Fix test

5e899f5

nealrichardson force-pushed the r-s3 branch from 2fc8836 to 5e899f5 Compare April 15, 2020 20:42

nealrichardson added 2 commits April 15, 2020 13:43

Turn off windows test verbosity

5c04d42

Skip test on windows (not worth debugging)

3a40da4

fsaintjacques self-requested a review April 16, 2020 17:36

fsaintjacques added 2 commits April 16, 2020 16:03

Fix S3FileSystem

c794511

Improve S3 tests speed

9e5282d

fsaintjacques approved these changes Apr 16, 2020

View reviewed changes

nealrichardson commented Apr 16, 2020

View reviewed changes

ci/scripts/cpp_test.sh Show resolved Hide resolved

ci/scripts/r_test.sh Show resolved Hide resolved

Add comments to explain AWS env var

3f49da9

nealrichardson closed this in 9197ba8 Apr 16, 2020

nealrichardson deleted the r-s3 branch April 16, 2020 23:09

asfimport mentioned this pull request Apr 16, 2020

[R] Implement S3 file-system interface in R #22809

Closed

ARROW-6439: [R] Implement S3 file-system interface in R #6901

ARROW-6439: [R] Implement S3 file-system interface in R #6901

Uh oh!

Conversation

nealrichardson commented Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 11, 2020

Uh oh!

fsaintjacques left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fsaintjacques commented Apr 16, 2020

Uh oh!

nealrichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nealrichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nealrichardson commented Apr 11, 2020 •

edited

Loading