Copilot commented on code in PR #50128:
URL: https://github.com/apache/arrow/pull/50128#discussion_r3401465185
##########
cpp/src/arrow/filesystem/s3fs.h:
##########
@@ -290,6 +291,22 @@ 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 or are better kept out of it (such as credentials).
+ /// Each option is a (name, value) pair. Recognized keys:
+ /// - "access_key" (std::string)
+ /// - "secret_key" (std::string)
+ /// - "session_token" (std::string)
+ /// - "retry_strategy" (std::shared_ptr<S3RetryStrategy>)
+ /// Options take precedence over the URI; unknown keys or invalid values
return
+ /// Status::Invalid.
Review Comment:
The FromUriAndOptions() doc comment lists recognized option keys, but it
omits the newly supported "default_metadata" option handled in s3fs.cc. This
makes the public API documentation inaccurate.
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -327,7 +327,60 @@ S3Options S3Options::FromAssumeRoleWithWebIdentity() {
}
Result<S3Options> S3Options::FromUri(const Uri& uri, std::string* out_path) {
- S3Options options;
+ return FromUriAndOptions(uri, FileSystemFactoryOptions{}, out_path);
+}
+
+Result<S3Options> S3Options::FromUri(const std::string& uri_string,
+ std::string* out_path) {
+ Uri uri;
+ RETURN_NOT_OK(uri.Parse(uri_string));
+ return FromUri(uri, out_path);
+}
+
+namespace {
+
+template <typename T>
+Result<T> GetOption(const std::string& key, const std::any& value) {
+ if (const auto* v = std::any_cast<T>(&value)) {
+ return *v;
+ }
+ return Status::Invalid("S3 filesystem option '", key, "' has the wrong
type");
+}
+
+} // namespace
+
+Result<S3Options> S3Options::FromUriAndOptions(const ::arrow::util::Uri& uri,
+ const FileSystemFactoryOptions&
options,
+ std::string* out_path) {
+ std::optional<std::string> access_key, secret_key, session_token;
+ std::shared_ptr<S3RetryStrategy> retry_strategy;
+ std::shared_ptr<const KeyValueMetadata> default_metadata;
+ for (const auto& [key, value] : options) {
+ if (key == "access_key") {
+ ARROW_ASSIGN_OR_RAISE(access_key, GetOption<std::string>(key, value));
+ } else if (key == "secret_key") {
+ ARROW_ASSIGN_OR_RAISE(secret_key, GetOption<std::string>(key, value));
+ } else if (key == "session_token") {
+ ARROW_ASSIGN_OR_RAISE(session_token, GetOption<std::string>(key, value));
+ } else if (key == "retry_strategy") {
+ ARROW_ASSIGN_OR_RAISE(retry_strategy,
+ GetOption<std::shared_ptr<S3RetryStrategy>>(key,
value));
+ } else if (key == "default_metadata") {
+ ARROW_ASSIGN_OR_RAISE(default_metadata,
+ GetOption<std::shared_ptr<KeyValueMetadata>>(key,
value));
+ } else {
Review Comment:
The "default_metadata" option is stored as std::shared_ptr<const
KeyValueMetadata>, but the current any_cast expects
std::shared_ptr<KeyValueMetadata>. Because std::any requires an exact type
match, callers passing a std::shared_ptr<const KeyValueMetadata> will get a
spurious "wrong type" error. Consider accepting both const and non-const
shared_ptr forms.
--
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]
