Copilot commented on code in PR #50128:
URL: https://github.com/apache/arrow/pull/50128#discussion_r3395469395
##########
cpp/src/arrow/filesystem/filesystem.cc:
##########
@@ -893,20 +893,24 @@ Status LoadFileSystemFactories(const char* libpath) {
namespace {
-Result<std::shared_ptr<FileSystem>> FileSystemFromUriReal(const Uri& uri,
- const std::string&
uri_string,
- const io::IOContext&
io_context,
- std::string*
out_path) {
+Result<std::shared_ptr<FileSystem>> FileSystemFromUriReal(
+ const Uri& uri, const std::string& uri_string,
+ const FileSystemFactoryOptions& options, const io::IOContext& io_context,
+ std::string* out_path) {
const auto scheme = uri.scheme();
{
ARROW_ASSIGN_OR_RAISE(
auto* factory,
FileSystemFactoryRegistry::GetInstance()->FactoryForScheme(scheme));
if (factory != nullptr) {
- return factory->function(uri, io_context, out_path);
+ return factory->function(uri, options, io_context, out_path);
}
}
+ if (!options.empty()) {
+ return Status::NotImplemented("Filesystem options are not supported yet
for scheme '",
+ scheme, "', got ", options.size(), "
option(s)");
+ }
Review Comment:
The early `!options.empty()` check returns a generic NotImplemented error
before scheme-specific blocks run, which can mask more accurate errors (e.g.
for `abfs`/`gs`/`hdfs` when Arrow is compiled without the corresponding
support). Consider preserving the existing scheme-specific NotImplemented
messages for those unsupported builds even when options are provided.
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -290,6 +291,20 @@ struct ARROW_EXPORT S3Options {
std::string* out_path = NULLPTR);
static Result<S3Options> FromUri(const std::string& uri,
std::string* out_path = NULLPTR);
+
+ /// Equivalent to FromUri() with specific backend options that can't be
represented
+ /// on the URI overlaid on top. Recognized keys:
+ /// - "access_key" (std::string)
+ /// - "secret_key" (std::string)
+ /// - "session_token" (std::string).
+ /// Options take precedence over the URI; unknown keys return
+ /// Status::Invalid.
Review Comment:
This docstring is misleading: the recognized keys here are primarily about
supplying/overriding credentials out-of-band (even though credentials *can*
also be expressed in the URI). Rewording would better match the actual behavior
and intent.
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -3603,11 +3652,18 @@ Result<std::string> ResolveS3BucketRegion(const
std::string& bucket) {
auto kS3FileSystemModule = ARROW_REGISTER_FILESYSTEM(
"s3",
- [](const arrow::util::Uri& uri, const io::IOContext& io_context,
+ [](const arrow::util::Uri& uri, const FileSystemFactoryOptions& options,
+ const io::IOContext& io_context,
std::string* out_path) -> Result<std::shared_ptr<fs::FileSystem>> {
+ /*if (!options.empty()) {
+ return Status::NotImplemented(
+ "S3 filesystem factory options are not supported yet, got: ",
options.size(),
+ " option(s)");
+ }*/
RETURN_NOT_OK(EnsureS3Initialized());
Review Comment:
Remove the leftover commented-out options rejection block. It is dead code
now that the S3 factory consumes `options` and will reject unknown keys via
`S3Options::FromUriAndOptions`.
--
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]
