Uh oh!
There was an error while loading. Please reload this page.
ARROW-9869: [R] Implement full S3FileSystem/S3Options constructor - #8197
ARROW-9869: [R] Implement full S3FileSystem/S3Options constructor#8197nealrichardson wants to merge 13 commits into
Conversation
5864fbf to
425923dComparenealrichardson
commented
Sep 17, 2020
@github-actions crossbow submit homebrew-r-autobrew |
Revision: 425923d986bf4bd9e7950b9dd5dced70ddc1161f Submitted crossbow builds: ursa-labs/crossbow @ actions-545
|
nealrichardson
commented
Sep 17, 2020
@github-actions crossbow submit homebrew-r-autobrew |
Revision: 2b89af19be38e0756af0c1fbb4f066555dc22d15 Submitted crossbow builds: ursa-labs/crossbow @ actions-546
|
nealrichardson
commented
Sep 17, 2020
@github-actions crossbow submit homebrew-r-autobrew |
Revision: 51b93dfc54b3b5f9a33e72d7e3009afd16825975 Submitted crossbow builds: ursa-labs/crossbow @ actions-547
|
There was a problem hiding this comment.
Why not reuse get_optional_string here?
There was a problem hiding this comment.
Because I don't want to set it if the user doesn't provide it (i.e. if it is NULL). Or is empty string "" the same thing? s3fs.h doesn't give these fields default values.
There was a problem hiding this comment.
The empty string is implicitly the default value.
5180a51 to
1933a40Comparecfbd63d to
1e14daeCompare| #' @export | ||
| read_ipc_stream <- function(file, as_data_frame = TRUE, ...) { | ||
| read_ipc_stream <- function(file, as_data_frame = TRUE, filesystem = NULL, ...) { | ||
| if (!inherits(file, "InputStream")) { |
There was a problem hiding this comment.
There are a lot of this happening. Does this really need file= and filesystem. Can this be something like:
read_ipc_stream(filesystem$open(file))?
There was a problem hiding this comment.
Unfortunately, it's spelled filesystem$OpenInputStream(file) if it's for reading, or filesystem$OpenOutputStream(file) for writing. I resisted adding the extra argument, but I thought it was less bad than requiring users to learn those.
But we could think of other solutions. One possibility is having some sort of FileLocator object that is filesystem + path; we've started exploring that in places in the C++ library, and I've been looking into similar things for ARROW-9870. Maybe we could revisit this question in that PR (next week-ish)?
nealrichardson
commented
Sep 25, 2020
I'm going to merge this and we can follow up with UX questions in the next PR. |
Also adds tests that run on minio, which I've tested locally, added documentation for how to run them, and added them to the macOS ("autobrew") nightly test run (because the Linux builds still don't have aws-sdk-cpp support).