GH-46369: [C++] Add key-value pairs to the FileSystemFactory interface and to S3Options

[raulcd]

Rationale for this change

From the issue description:

From discussion on PR #41559 (comment) and the mailing list, it's desirable to extend the filesystem factory interface to accept a set of key-value pairs in addition to a URL. This will draw a clearer distinction between filesystem options which are expected to be independent of the user (the URI, which should be reusable by multiple users) and options which are expected to be particular to a user (the key-value pairs, which may contain a user's keys/tokens/...). In particular it will remove the ambiguity of whether filesystem URIs must be guarded: URIs will not contain secrets and need not be guarded.

What changes are included in this PR?

• A new options FileSystemFactoryOptions, added to FileSystemFromUriAndOptions which allows to pass via FileSystemFromUriReal through FileSystemFactory::function
• The s3 and local/file factories are migrated to the new 4-arg signature.
• Schemes that do not yet consume options reject non-empty options with NotImplemented rather than silently dropping them.
• examplefs now reads two typed options and appends them to the output path, exercised by arrow-filesystem-test, proving std::any survives the libarrow to arrow_filesystem_example.so boundary.
• CI: ci/scripts/cpp_build.sh now forwards ARROW_S3_MODULE to CMake. The conda-cpp image was already setting ARROW_S3_MODULE=ON, but the flag was never passed through, so s3fs_module_test (added in GH-40343: [C++] Move S3FileSystem to the registry #41559) had not actually been built or run in CI. This activates it.
• Added access_key, secret_key, session_token, retry_strategy and default_metadata as FileSystemFactoryOptions on S3Options::FromUriAndOptions.
• Tests exercising

Are these changes tested?

Yes, new tests added to validate a dynamically-loaded filesystem is able to receive options via std::any and tests validating rejection if options passes to Filesystems that don't support it. Tests also added for S3Options::FromUriAndOptions receiving the new FileSystemFactoryOptions and usage on separate s3fs module

Are there any user-facing changes?

New public FileSystemFromUriOptions taking an options list. Existing ones are unchanged.

• GitHub Issue: [C++] Add key-value pairs to the FileSystemFactory interface #46369

[Copilot]

Pull request overview

This PR extends the C++ filesystem factory plumbing to accept an additional set of backend-specific key/value options (in addition to the filesystem URI), aligning with GH-46369’s goal of separating reusable URIs from user-specific configuration.

Changes:

• Extends FileSystemFactory to accept options and adds compatibility construction for existing factories.
• Adds new FileSystemFromUri overloads that accept options, and threads them through the factory registry call path.
• Updates registered filesystem modules/tests (examplefs, S3, localfs tests) to compile with the new factory signature.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
cpp/src/arrow/testing/examplefs.cc	Updates example filesystem factory signature and forwards options to nested FileSystemFromUri call.
cpp/src/arrow/filesystem/s3fs.cc	Updates S3 registered factory signature to include options (currently unused).
cpp/src/arrow/filesystem/localfs_test.cc	Disambiguates nullptr factory registrations under the new overloaded FileSystemFactory constructors.
cpp/src/arrow/filesystem/filesystem.h	Introduces options-aware FileSystemFactory + new FileSystemFromUri overloads and documentation.
cpp/src/arrow/filesystem/filesystem.cc	Implements the new FileSystemFromUri overloads and forwards options into the registry-based factory dispatch.

Comments suppressed due to low confidence (1)

cpp/src/arrow/filesystem/filesystem.cc:925

• The new options argument is only forwarded to registered factories; built-in scheme handling (abfs/gcs/hdfs/mock) ignores it entirely. This makes FileSystemFromUri(..., options, ...) silently drop backend-specific options for these schemes. Consider either (a) plumbing options into the built-in scheme option parsing / constructors, or (b) rejecting non-empty options for schemes that don’t support them yet to avoid surprising behavior.

Result<std::shared_ptr<FileSystem>> FileSystemFromUriReal(
    const Uri& uri, const std::string& uri_string,
    const std::vector<std::pair<std::string, std::any>>& 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, options, io_context, out_path);
    }
  }

  if (scheme == "abfs" || scheme == "abfss") {
#ifdef ARROW_AZURE
    ARROW_ASSIGN_OR_RAISE(auto options, AzureOptions::FromUri(uri, out_path));
    return AzureFileSystem::Make(options, io_context);
#else
    return Status::NotImplemented(
        "Got Azure Blob File System URI but Arrow compiled without Azure Blob File "
        "System support");
#endif
  }
  if (scheme == "gs" || scheme == "gcs") {
#ifdef ARROW_GCS
    ARROW_ASSIGN_OR_RAISE(auto options, GcsOptions::FromUri(uri, out_path));
    return GcsFileSystem::Make(options, io_context);
#else

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[raulcd]

@pitrou @kou I've started working on this to hopefully move S3 filesystem out of libarrow and into its own shared library.
About the scope of this PR, would you push this plumbing PR in isolation (in which case I would add a couple of tests to validate options are passed), or should I add some S3 specific options as part of this PR?

For the scope of the S3 options (on this PR or a follow up one), would you mimic all S3Options as part of the new k/v pair or only a small subset, example initially only region and credentials?

[raulcd]

@pitrou any thoughts about the scope for this PR, as shared above:

About the scope of this PR, would you push this plumbing PR in isolation (in which case I would add a couple of tests to validate options are passed), or should I add some S3 specific options as part of this PR?

For the scope of the S3 options (on this PR or a follow up one), would you mimic all S3Options as part of the new k/v pair or only a small subset, example initially only region and credentials?

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

[kou]

How many S3 specific options do we have?
If we have only few options, how about implementing them in this PR for easy to review with real use case?
If we have many options, how about implementing only a few options in this PR and implementing others in a follow-up PR for easy to review?

BTW, do we need to use std::any? Can we reuse existing arrow::KeyValueMetadata?

[raulcd]

Thanks @kou for the review, I really appreciate you taking the time!

How many S3 specific options do we have?

I counted around 13 that we probably don't want to express as part of the URI (credentials, assume-role params, custom providers, connect/request timeouts, a few behavior flags, default_metadata, retry_strategy, sse_customer_key).

If we have only few options, how about implementing them in this PR for easy to review with real use case? If we have many options, how about implementing only a few options in this PR and implementing others in a follow-up PR for easy to review?

I can do that, I basically started a follow up PR on top of this one where I started working towards adding them:

• GH-50126: [C++] Make S3Filesystem consume key-value options #50128

I wanted to have this in isolation for easier initial review but I am happy to push a single PR or add some of those cases here.

BTW, do we need to use std::any? Can we reuse existing arrow::KeyValueMetadata?

The std::any case is what @pitrou proposed on the mailing list and basically was to be able to cater for S3Options that are not string-encodable like retry_strategy with std::shared_ptr<S3RetryStrategy> , or default_metadata std::shared_ptr<const KeyValueMetadata>.

There's also credentials_provider std::shared_ptr<Aws::Auth::AWSCredentialsProvider> .

The cost is the type-erasure across the shared-library boundary, which I am trying to validate it won't be an issue for complex types on the other PR I pointed above. I've only tested std::string on Linux so far as I just started working on it. I should validate it with complex types and not only on Linux but also on other OS, as I've read cases where this might not behave as expected across DLL boundaries.

But now that I think more about the added complexity, I am unsure it is worth the hassle because:

• credentials_provider won't be able to be built from libarrow without AWS headers (needs the SDK)
• default_metadata is string-encodable (it's a str:str).
• S3RetryStrategy is the only thing std::any buys us: letting a libarrow-only caller pass a custom S3RetryStrategy subclass, but only if we keep that interface in libarrow which I am unsure we will as this should be part of the s3fs module. We could come up with string encodable retry_strategy=standard, retry_max_attempts=5

@pitrou @kou thoughts about that?

[pitrou]

std::any will be easier to use for the caller than have to string-serialize options in a KeyValueMetadata.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

[kou]

std::any will be easier to use for the caller than have to string-serialize options in a KeyValueMetadata.

Can std::any be easier to use for our bindings? If so, I don't object the std::any approach.

[pitrou]

KeyValueMetadata is meant for metadata serialized using Flatbuffers (or Thrift etc.).

I don't think it's a good idea to use it for things that do not get serialized. It imposes a string parsing or conversion, loses typing etc.

[raulcd]

Can std::any be easier to use for our bindings? If so, I don't object the std::any approach.

Hi @kou I wasn't sure so I decided to take some time to investigate it. I've created this commit (acfc939) on top of this PR to test std::any bindings on R, Python and C GLib / Ruby. I've used Claude to help with the Ruby and R bindings but I've validated the test is exercised by producing a failing test first (TDD). All of the binding usages for std::any look quite ergonomic and easy to use. I've created a PR to exercise CI and demonstrate the bindings are green, can be seen here:

• DRAFT: PR To validate ergonomy of bindings for std::any #50151

So, in my opinion it is easy to use for our bindings, what are your thoughts on it?

Note: A minor nit, we seem to have an arrow::dataset::FileSystemFactoryOptions and the new arrow::fs::FileSystemFactoryOptions forced me to alias on the Python bindings, maybe we want to rename the new one as this is the right time but not a big deal if we have to keep an alias.

[kou]

Thanks for trying it. OK. Let's use std::any.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

[raulcd]

@kou @pitrou I decided to add some of the S3Options to this PR for easier review as a global mechanism in action and not only the capability. I've moved this from draft to review. Let me know what you think.

[raulcd]

@bkietz if you happen to have some spare cycles and can take a look

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.