cboettig opened a new pull request, #33918:
URL: https://github.com/apache/arrow/pull/33918
`s3_bucket` has needless conditional logic that defaulted to filesystem
construction using `FileSystem$from_uri`, and only falling back on
`S3FileSystem$create()` when additional optional arguments were supplied.
This approach is more verbose opaque, and not properly documented, leading
to errors in numerous edge cases described below. All of these problems are
easily avoided by using the `S3FileSystem$create()` constructor directly,
(which also corresponds to the documentation for the S3 Filesytem in pyarrow).
As you know, the S3 SDK is capable of reading user keys, secret keys,
region, endpoint, and scheme from environmental variables and from `.aws`
config files, and users who have encountered the S3 system in other open source
software are very familiar with these uses.
The current implementation of `s3_bucket` needlessly seeks to construct the
`s3://` URI notation instead,
`s3://{access_key}:{secret_key}@?scheme={scheme}&endpoint_override={uri_escaped_enpoint}`
and create the filesystem object using `FileSystem$from_uri`:
https://github.com/apache/arrow/blob/4dd5cedb21d7b58d837bdb3c0d35a5cd80fd9f4b/r/R/filesystem.R#L463
This is not the correct URI construction, as it ignores additional options
such as key, secret key, scheme, and endpoint override (and possibly others)
that the user has passed to the s3_bucket() function. This creates errors and
unpredictable behavior to the user -- for instance observe that:
```r
s3 <- s3_bucket("neon4cast-scores",
endpoint_override="data.ecoforecast.org", anonymous=TRUE)
```
Works as anticipated -- but only because the exact same bucket-name
requested from the MINIO-based https://data.ecoforecast.org/ , happens to also
exist on Amazon's default endpoint, s3.amazonaws.com (allowing the `from_uri()`
method to erroneously succeed, connecting to the wrong endpoint, and then be
replaced by a connection to the correct endpoint triggered by the presence of
`...` . (It took me some time to track down this behavior, as I and many
colleagues have used `s3_bucket()` with independent endpoints for over a year,
not realizing why some bucket names would mysteriously fail while others
worked).
I do not think this problem is isolated to the use of `endpoint_override`, I
don't think that `from_uri` construction is preferable to
`S3FileSystem$create()` even when a user is working against the default
s3.awsamazon.com endpoints, and at any event, requires more care that the URI
is constructed with respect to user-facing options (including respecting
`AWS_REGION` as documented in the SDK. I believe this would address
https://github.com/apache/arrow/issues/33904, but emphasize that _no special
conditional handling_ is needed here to support endpoint_override or other
options -- the AWS SDK is already well designed to handle alternative endpoints
and all of these other options just fine, we just let the standard
S3FileSystem$create method do it's job.
I think the change I propose here would make the code base robust to these
edge cases, improve performance by decreasing redundant API calls, and requires
no change to any of the documentation or function API.
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's
Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing
Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor
PR](https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#Minor-Fixes).
Could you open an issue for this pull request on GitHub?
https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the
[Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.)
of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in
the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your
changes and offer better suggestions for fixes.
-->
### What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is
sometimes worth providing a summary of the individual changes in this PR.
-->
### Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are
they covered by existing tests)?
-->
### Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line
below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix
either (a) a security vulnerability, (b) a bug that caused incorrect or invalid
data to be produced, or (c) a bug that causes a crash (even when the API
contract is upheld). We use this to highlight fixes to issues that may affect
users without their knowledge. For this reason, fixing bugs that cause errors
don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
