Skip to content

Commit 591c14b

Browse files
raulcdpitrou
andauthored
GH-46369: [C++] Add key-value pairs to the FileSystemFactory interface and to S3Options (#50044)
### Rationale for this change From the issue description: > From discussion on PR #41559 (comment) and the [mailing list](https://lists.apache.org/thread/ot4p23mc292p5103nfgrbyqpnhhp6clx), 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 #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: #46369 Lead-authored-by: Raúl Cumplido <raulcumplido@gmail.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
1 parent f0d3f17 commit 591c14b

13 files changed

Lines changed: 506 additions & 43 deletions

File tree

ci/scripts/cpp_build.sh

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ if [ "${ARROW_ENABLE_THREADING:-ON}" = "OFF" ]; then
7070
ARROW_JEMALLOC=OFF
7171
ARROW_MIMALLOC=OFF
7272
ARROW_S3=OFF
73+
ARROW_S3_MODULE=OFF
7374
ARROW_WITH_OPENTELEMETRY=OFF
7475
fi
7576

@@ -229,6 +230,7 @@ else
229230
-DARROW_PARQUET=${ARROW_PARQUET:-OFF} \
230231
-DARROW_RUNTIME_SIMD_LEVEL=${ARROW_RUNTIME_SIMD_LEVEL:-MAX} \
231232
-DARROW_S3=${ARROW_S3:-OFF} \
233+
-DARROW_S3_MODULE=${ARROW_S3_MODULE:-OFF} \
232234
-DARROW_SIMD_LEVEL=${ARROW_SIMD_LEVEL:-DEFAULT} \
233235
-DARROW_SUBSTRAIT=${ARROW_SUBSTRAIT:-OFF} \
234236
-DARROW_TEST_LINKAGE=${ARROW_TEST_LINKAGE:-shared} \

cpp/src/arrow/filesystem/CMakeLists.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ if(ARROW_S3)
128128
endif()
129129
endif()
130130

131-
if(ARROW_S3_MODULE)
131+
if(ARROW_S3_MODULE AND ARROW_BUILD_TESTS)
132132
add_arrow_test(s3fs_module_test
133133
SOURCES
134134
s3fs_module_test.cc

cpp/src/arrow/filesystem/filesystem.cc

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -893,20 +893,24 @@ Status LoadFileSystemFactories(const char* libpath) {
893893

894894
namespace {
895895

896-
Result<std::shared_ptr<FileSystem>> FileSystemFromUriReal(const Uri& uri,
897-
const std::string& uri_string,
898-
const io::IOContext& io_context,
899-
std::string* out_path) {
896+
Result<std::shared_ptr<FileSystem>> FileSystemFromUriReal(
897+
const Uri& uri, const std::string& uri_string,
898+
const FileSystemFactoryOptions& options, const io::IOContext& io_context,
899+
std::string* out_path) {
900900
const auto scheme = uri.scheme();
901901

902902
{
903903
ARROW_ASSIGN_OR_RAISE(
904904
auto* factory,
905905
FileSystemFactoryRegistry::GetInstance()->FactoryForScheme(scheme));
906906
if (factory != nullptr) {
907-
return factory->function(uri, io_context, out_path);
907+
return factory->function(uri, options, io_context, out_path);
908908
}
909909
}
910+
if (!options.empty()) {
911+
return Status::NotImplemented("Filesystem options are not supported yet for scheme '",
912+
scheme, "', got ", options.size(), " option(s)");
913+
}
910914

911915
if (scheme == "abfs" || scheme == "abfss") {
912916
#ifdef ARROW_AZURE
@@ -962,14 +966,28 @@ Result<std::shared_ptr<FileSystem>> FileSystemFromUriReal(const Uri& uri,
962966

963967
Result<std::shared_ptr<FileSystem>> FileSystemFromUri(const std::string& uri_string,
964968
std::string* out_path) {
965-
return FileSystemFromUri(uri_string, io::default_io_context(), out_path);
969+
return FileSystemFromUriAndOptions(uri_string, /*options=*/{}, io::default_io_context(),
970+
out_path);
971+
}
972+
973+
Result<std::shared_ptr<FileSystem>> FileSystemFromUriAndOptions(
974+
const std::string& uri_string, const FileSystemFactoryOptions& options,
975+
std::string* out_path) {
976+
return FileSystemFromUriAndOptions(uri_string, options, io::default_io_context(),
977+
out_path);
966978
}
967979

968980
Result<std::shared_ptr<FileSystem>> FileSystemFromUri(const std::string& uri_string,
969981
const io::IOContext& io_context,
970982
std::string* out_path) {
983+
return FileSystemFromUriAndOptions(uri_string, /*options=*/{}, io_context, out_path);
984+
}
985+
986+
Result<std::shared_ptr<FileSystem>> FileSystemFromUriAndOptions(
987+
const std::string& uri_string, const FileSystemFactoryOptions& options,
988+
const io::IOContext& io_context, std::string* out_path) {
971989
ARROW_ASSIGN_OR_RAISE(auto fsuri, ParseFileSystemUri(uri_string));
972-
return FileSystemFromUriReal(fsuri, uri_string, io_context, out_path);
990+
return FileSystemFromUriReal(fsuri, uri_string, options, io_context, out_path);
973991
}
974992

975993
Result<std::shared_ptr<FileSystem>> FileSystemFromUriOrPath(const std::string& uri_string,

cpp/src/arrow/filesystem/filesystem.h

Lines changed: 85 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
#pragma once
1919

20+
#include <any>
2021
#include <chrono>
2122
#include <cstdint>
2223
#include <functional>
@@ -28,6 +29,7 @@
2829

2930
#include "arrow/filesystem/type_fwd.h"
3031
#include "arrow/io/interfaces.h"
32+
#include "arrow/result.h"
3133
#include "arrow/type_fwd.h"
3234
#include "arrow/util/compare.h"
3335
#include "arrow/util/macros.h"
@@ -357,13 +359,44 @@ class ARROW_EXPORT FileSystem
357359
bool default_async_is_sync_ = true;
358360
};
359361

362+
using FileSystemFactoryOptions = std::vector<std::pair<std::string, std::any>>;
363+
360364
struct FileSystemFactory {
361365
std::function<Result<std::shared_ptr<FileSystem>>(
362-
const Uri& uri, const io::IOContext& io_context, std::string* out_path)>
366+
const Uri& uri, const FileSystemFactoryOptions& options,
367+
const io::IOContext& io_context, std::string* out_path)>
363368
function;
364369
std::string_view file;
365370
int line;
366371

372+
/// Construct from an options-aware factory function.
373+
FileSystemFactory(std::function<Result<std::shared_ptr<FileSystem>>(
374+
const Uri&, const FileSystemFactoryOptions&, const io::IOContext&,
375+
std::string*)>
376+
fn,
377+
std::string_view file, int line)
378+
: function(std::move(fn)), file(file), line(line) {}
379+
380+
/// Construct from a non-options aware factory function maintaining source compatibility
381+
/// with existing factories.
382+
FileSystemFactory(std::function<Result<std::shared_ptr<FileSystem>>(
383+
const Uri&, const io::IOContext&, std::string*)>
384+
fn,
385+
std::string_view file, int line)
386+
: function([fn = std::move(fn)](
387+
const Uri& uri, const FileSystemFactoryOptions& options,
388+
const io::IOContext& ctx,
389+
std::string* out_path) -> Result<std::shared_ptr<FileSystem>> {
390+
if (!options.empty()) {
391+
return Status::NotImplemented(
392+
"Filesystem factory does not support additional options, got ",
393+
options.size(), " option(s)");
394+
}
395+
return fn(uri, ctx, out_path);
396+
}),
397+
file(file),
398+
line(line) {}
399+
367400
bool operator==(const FileSystemFactory& other) const {
368401
// In the case where libarrow is linked statically both to the executable and to a
369402
// dynamically loaded filesystem implementation library, the library contains a
@@ -536,7 +569,7 @@ void EnsureFinalized();
536569
/// \brief Create a new FileSystem by URI
537570
///
538571
/// Recognized schemes are "file", "mock", "hdfs", "viewfs", "s3",
539-
/// "gs" and "gcs".
572+
/// "gs", "gcs", "abfs" and "abfss".
540573
///
541574
/// Support for other schemes can be added using RegisterFileSystemFactory.
542575
///
@@ -547,10 +580,34 @@ ARROW_EXPORT
547580
Result<std::shared_ptr<FileSystem>> FileSystemFromUri(const std::string& uri,
548581
std::string* out_path = NULLPTR);
549582

583+
/// \brief Create a new FileSystem by URI with extended backend-specific filesystem
584+
/// options
585+
///
586+
/// Recognized schemes are "file", "mock", "hdfs", "viewfs", "s3",
587+
/// "gs", "gcs", "abfs" and "abfss".
588+
///
589+
/// Support for other schemes can be added using RegisterFileSystemFactory.
590+
///
591+
/// \param[in] uri the URI to give access to
592+
/// \param[in] options a list of backend-specific filesystem options
593+
/// Each option is a (name, value) pair.
594+
/// The expected type is specific to the backend and
595+
/// option name.
596+
/// Options are forwarded to schemes dispatched through a registered
597+
/// FileSystemFactory. Non-empty options return NotImplemented for a registered
598+
/// FileSystemFactory that does not support them or for schemes not handled by
599+
/// a registered factory.
600+
/// \param[out] out_path (optional) Path inside the filesystem.
601+
/// \return out_fs FileSystem instance.
602+
ARROW_EXPORT
603+
Result<std::shared_ptr<FileSystem>> FileSystemFromUriAndOptions(
604+
const std::string& uri, const FileSystemFactoryOptions& options,
605+
std::string* out_path = NULLPTR);
606+
550607
/// \brief Create a new FileSystem by URI with a custom IO context
551608
///
552609
/// Recognized schemes are "file", "mock", "hdfs", "viewfs", "s3",
553-
/// "gs" and "gcs".
610+
/// "gs", "gcs", "abfs" and "abfss".
554611
///
555612
/// Support for other schemes can be added using RegisterFileSystemFactory.
556613
///
@@ -563,6 +620,31 @@ Result<std::shared_ptr<FileSystem>> FileSystemFromUri(const std::string& uri,
563620
const io::IOContext& io_context,
564621
std::string* out_path = NULLPTR);
565622

623+
/// \brief Create a new FileSystem by URI with a custom IO context with backend-specific
624+
/// filesystem options
625+
///
626+
/// Recognized schemes are "file", "mock", "hdfs", "viewfs", "s3",
627+
/// "gs", "gcs", "abfs" and "abfss".
628+
///
629+
/// Support for other schemes can be added using RegisterFileSystemFactory.
630+
///
631+
/// \param[in] uri a URI-based path, ex: file:///some/local/path
632+
/// \param[in] options a list of backend-specific filesystem options
633+
/// Each option is a (name, value) pair.
634+
/// The expected type is specific to the backend and
635+
/// option name.
636+
/// Options are forwarded to schemes dispatched through a registered
637+
/// FileSystemFactory. Non-empty options return NotImplemented for a registered
638+
/// FileSystemFactory that does not support them or for schemes not handled by
639+
/// a registered factory.
640+
/// \param[in] io_context an IOContext which will be associated with the filesystem
641+
/// \param[out] out_path (optional) Path inside the filesystem.
642+
/// \return out_fs FileSystem instance.
643+
ARROW_EXPORT
644+
Result<std::shared_ptr<FileSystem>> FileSystemFromUriAndOptions(
645+
const std::string& uri, const FileSystemFactoryOptions& options,
646+
const io::IOContext& io_context, std::string* out_path = NULLPTR);
647+
566648
/// \brief Create a new FileSystem by URI
567649
///
568650
/// Support for other schemes can be added using RegisterFileSystemFactory.

cpp/src/arrow/filesystem/filesystem_test.cc

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
// specific language governing permissions and limitations
1616
// under the License.
1717

18+
#include <any>
1819
#include <memory>
1920
#include <string>
2021
#include <utility>
@@ -640,6 +641,11 @@ TEST_F(TestMockFS, FileSystemFromUri) {
640641
Invalid, ::testing::HasSubstr("syntax error at character ' ' (position 12)"),
641642
FileSystemFromUri("mock:/folder name/bar", &path));
642643
CheckDirs({});
644+
FileSystemFactoryOptions options{{"some_option", 1}};
645+
EXPECT_RAISES_WITH_MESSAGE_THAT(
646+
NotImplemented, ::testing::HasSubstr("options are not supported"),
647+
FileSystemFromUriAndOptions("mock:///foo/bar", options, &path));
648+
CheckDirs({});
643649
}
644650

645651
////////////////////////////////////////////////////////////////////////////

cpp/src/arrow/filesystem/localfs_test.cc

Lines changed: 63 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
// specific language governing permissions and limitations
1616
// under the License.
1717

18+
#include <any>
1819
#include <memory>
1920
#include <sstream>
2021
#include <string>
@@ -29,6 +30,7 @@
2930
#include "arrow/filesystem/path_util.h"
3031
#include "arrow/filesystem/test_util.h"
3132
#include "arrow/filesystem/util_internal.h"
33+
#include "arrow/testing/examplefs.h"
3234
#include "arrow/testing/gtest_util.h"
3335
#include "arrow/testing/matchers.h"
3436
#include "arrow/util/io_util.h"
@@ -83,6 +85,12 @@ Result<std::shared_ptr<FileSystem>> FSFromUriOrPath(const std::string& uri,
8385
////////////////////////////////////////////////////////////////////////////
8486
// Registered FileSystemFactory tests
8587

88+
struct ConcreteTypedOption : ExampleTypedOption {
89+
explicit ConcreteTypedOption(int value) : value_(value) {}
90+
int value() const override { return value_; }
91+
int value_;
92+
};
93+
8694
class SlowFileSystemPublicProps : public SlowFileSystem {
8795
public:
8896
SlowFileSystemPublicProps(std::shared_ptr<FileSystem> base_fs, double average_latency,
@@ -144,7 +152,6 @@ TEST(FileSystemFromUri, LoadedRegisteredFactory) {
144152
EXPECT_THAT(FileSystemFromUri("example:///hey/yo", &path), Raises(StatusCode::Invalid));
145153

146154
EXPECT_THAT(LoadFileSystemFactories(ARROW_FILESYSTEM_EXAMPLE_LIBPATH), Ok());
147-
148155
ASSERT_OK_AND_ASSIGN(auto fs, FileSystemFromUri("example:///hey/yo", &path));
149156
EXPECT_EQ(path, "/hey/yo");
150157
EXPECT_EQ(fs->type_name(), "local");
@@ -171,10 +178,21 @@ TEST(FileSystemFromUri, RuntimeRegisteredFactory) {
171178
}
172179

173180
FileSystemRegistrar kSegfaultFileSystemModule[]{
174-
ARROW_REGISTER_FILESYSTEM("segfault", nullptr, {}),
175-
ARROW_REGISTER_FILESYSTEM("segfault", nullptr, {}),
176-
ARROW_REGISTER_FILESYSTEM("segfault", nullptr, {}),
177-
};
181+
ARROW_REGISTER_FILESYSTEM(
182+
"segfault",
183+
std::function<Result<std::shared_ptr<FileSystem>>(
184+
const Uri&, const io::IOContext&, std::string*)>(nullptr),
185+
{}),
186+
ARROW_REGISTER_FILESYSTEM(
187+
"segfault",
188+
std::function<Result<std::shared_ptr<FileSystem>>(
189+
const Uri&, const io::IOContext&, std::string*)>(nullptr),
190+
{}),
191+
ARROW_REGISTER_FILESYSTEM(
192+
"segfault",
193+
std::function<Result<std::shared_ptr<FileSystem>>(
194+
const Uri&, const io::IOContext&, std::string*)>(nullptr),
195+
{})};
178196
TEST(FileSystemFromUri, LinkedRegisteredFactoryNameCollision) {
179197
// Since multiple registrars are defined in this translation unit which all
180198
// register factories for the 'segfault' scheme, using that scheme in FileSystemFromUri
@@ -185,6 +203,46 @@ TEST(FileSystemFromUri, LinkedRegisteredFactoryNameCollision) {
185203
// other schemes are not affected by the collision
186204
EXPECT_THAT(FileSystemFromUri("slowfile:///hey/yo", &path), Ok());
187205
}
206+
207+
TEST(FileSystemFromUriAndOptions, LoadedRegisteredFactory) {
208+
#ifdef __EMSCRIPTEN__
209+
GTEST_SKIP() << "Emscripten dynamic library testing disabled";
210+
#endif
211+
std::string path;
212+
EXPECT_THAT(LoadFileSystemFactories(ARROW_FILESYSTEM_EXAMPLE_LIBPATH), Ok());
213+
// Validate extra options are forwarded to the factory.
214+
FileSystemFactoryOptions options{
215+
{"example_option_string", std::string("example_value")},
216+
{"example_option_int", 42},
217+
{"example_typed_option",
218+
std::shared_ptr<ExampleTypedOption>(std::make_shared<ConcreteTypedOption>(12345))},
219+
};
220+
ASSERT_OK_AND_ASSIGN(auto fs,
221+
FileSystemFromUriAndOptions("example:///hey/yo", options, &path));
222+
EXPECT_EQ(path, "/hey/yo/example_value/42/12345");
223+
EXPECT_EQ(fs->type_name(), "local");
224+
}
225+
226+
TEST(FileSystemFromUriAndOptions, RuntimeRegisteredFactory) {
227+
std::string path;
228+
EXPECT_THAT(FileSystemFromUriAndOptions("slowfile3:///hey/yo", {}, &path),
229+
Raises(StatusCode::Invalid));
230+
231+
EXPECT_THAT(
232+
RegisterFileSystemFactory("slowfile3", {SlowFileSystemFactory, __FILE__, __LINE__}),
233+
Ok());
234+
235+
ASSERT_OK_AND_ASSIGN(auto fs,
236+
FileSystemFromUriAndOptions("slowfile3:///hey/yo", {}, &path));
237+
EXPECT_EQ(path, "/hey/yo");
238+
EXPECT_EQ(fs->type_name(), "slow");
239+
240+
// Validate that legacy (3-arg) factories reject non-empty options.
241+
FileSystemFactoryOptions unsupported{{"some_option", 1}};
242+
EXPECT_THAT(FileSystemFromUriAndOptions("slowfile3:///hey/yo", unsupported, &path),
243+
Raises(StatusCode::NotImplemented));
244+
}
245+
188246
////////////////////////////////////////////////////////////////////////////
189247
// Misc tests
190248

0 commit comments

Comments
 (0)