Skip to content

Commit ddac9df

Browse files
raulcdraulcd
committed
Several review fixes, rename to FileSystemFromUriAndOptions instead of FileSystemFromUri, do not silently ignore non-empty options if underlying FS does not support them and use FileSystemFactoryOptions instead of std::vector<std::pair<std::string, std::any>> everywhere
1 parent 94142b2 commit ddac9df

7 files changed

Lines changed: 43 additions & 37 deletions

File tree

‎cpp/src/arrow/filesystem/filesystem.cc‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -895,8 +895,8 @@ namespace {
895895

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

902902
{
@@ -966,25 +966,25 @@ Result<std::shared_ptr<FileSystem>> FileSystemFromUriReal(
966966

967967
Result<std::shared_ptr<FileSystem>> FileSystemFromUri(const std::string& uri_string,
968968
std::string* out_path) {
969-
return FileSystemFromUri(uri_string, /*options=*/{}, io::default_io_context(),
970-
out_path);
969+
return FileSystemFromUriAndOptions(uri_string, /*options=*/{}, io::default_io_context(),
970+
out_path);
971971
}
972972

973-
Result<std::shared_ptr<FileSystem>> FileSystemFromUri(
974-
const std::string& uri_string,
975-
const std::vector<std::pair<std::string, std::any>>& options, std::string* out_path) {
976-
return FileSystemFromUri(uri_string, options, io::default_io_context(), out_path);
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);
977978
}
978979

979980
Result<std::shared_ptr<FileSystem>> FileSystemFromUri(const std::string& uri_string,
980981
const io::IOContext& io_context,
981982
std::string* out_path) {
982-
return FileSystemFromUri(uri_string, /*options=*/{}, io_context, out_path);
983+
return FileSystemFromUriAndOptions(uri_string, /*options=*/{}, io_context, out_path);
983984
}
984985

985-
Result<std::shared_ptr<FileSystem>> FileSystemFromUri(
986-
const std::string& uri_string,
987-
const std::vector<std::pair<std::string, std::any>>& options,
986+
Result<std::shared_ptr<FileSystem>> FileSystemFromUriAndOptions(
987+
const std::string& uri_string, const FileSystemFactoryOptions& options,
988988
const io::IOContext& io_context, std::string* out_path) {
989989
ARROW_ASSIGN_OR_RAISE(auto fsuri, ParseFileSystemUri(uri_string));
990990
return FileSystemFromUriReal(fsuri, uri_string, options, io_context, out_path);

‎cpp/src/arrow/filesystem/filesystem.h‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -359,18 +359,20 @@ class ARROW_EXPORT FileSystem
359359
bool default_async_is_sync_ = true;
360360
};
361361

362+
using FileSystemFactoryOptions = std::vector<std::pair<std::string, std::any>>;
363+
362364
struct FileSystemFactory {
363365
std::function<Result<std::shared_ptr<FileSystem>>(
364-
const Uri& uri, const std::vector<std::pair<std::string, std::any>>& options,
366+
const Uri& uri, const FileSystemFactoryOptions& options,
365367
const io::IOContext& io_context, std::string* out_path)>
366368
function;
367369
std::string_view file;
368370
int line;
369371

370372
/// Construct from an options-aware factory function.
371373
FileSystemFactory(std::function<Result<std::shared_ptr<FileSystem>>(
372-
const Uri&, const std::vector<std::pair<std::string, std::any>>&,
373-
const io::IOContext&, std::string*)>
374+
const Uri&, const FileSystemFactoryOptions&, const io::IOContext&,
375+
std::string*)>
374376
fn,
375377
std::string_view file, int line)
376378
: function(std::move(fn)), file(file), line(line) {}
@@ -382,10 +384,15 @@ struct FileSystemFactory {
382384
fn,
383385
std::string_view file, int line)
384386
: function([fn = std::move(fn)](
385-
const Uri& uri,
386-
const std::vector<std::pair<std::string, std::any>>& /*ignored*/,
387+
const Uri& uri, const FileSystemFactoryOptions& options,
387388
const io::IOContext& ctx,
388-
std::string* out_path) { return fn(uri, ctx, out_path); }),
389+
std::string* out_path) -> Result<std::shared_ptr<FileSystem>> {
390+
if (!options.empty()) {
391+
return Status::NotImplemented(
392+
"This filesystem does not support additional options");
393+
}
394+
return fn(uri, ctx, out_path);
395+
}),
389396
file(file),
390397
line(line) {}
391398

@@ -593,8 +600,8 @@ Result<std::shared_ptr<FileSystem>> FileSystemFromUri(const std::string& uri,
593600
/// \param[out] out_path (optional) Path inside the filesystem.
594601
/// \return out_fs FileSystem instance.
595602
ARROW_EXPORT
596-
Result<std::shared_ptr<FileSystem>> FileSystemFromUri(
597-
const std::string& uri, const std::vector<std::pair<std::string, std::any>>& options,
603+
Result<std::shared_ptr<FileSystem>> FileSystemFromUriAndOptions(
604+
const std::string& uri, const FileSystemFactoryOptions& options,
598605
std::string* out_path = NULLPTR);
599606

600607
/// \brief Create a new FileSystem by URI with a custom IO context
@@ -635,8 +642,8 @@ Result<std::shared_ptr<FileSystem>> FileSystemFromUri(const std::string& uri,
635642
/// \param[out] out_path (optional) Path inside the filesystem.
636643
/// \return out_fs FileSystem instance.
637644
ARROW_EXPORT
638-
Result<std::shared_ptr<FileSystem>> FileSystemFromUri(
639-
const std::string& uri, const std::vector<std::pair<std::string, std::any>>& options,
645+
Result<std::shared_ptr<FileSystem>> FileSystemFromUriAndOptions(
646+
const std::string& uri, const FileSystemFactoryOptions& options,
640647
const io::IOContext& io_context, std::string* out_path = NULLPTR);
641648

642649
/// \brief Create a new FileSystem by URI

‎cpp/src/arrow/filesystem/filesystem_test.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -641,10 +641,10 @@ TEST_F(TestMockFS, FileSystemFromUri) {
641641
Invalid, ::testing::HasSubstr("syntax error at character ' ' (position 12)"),
642642
FileSystemFromUri("mock:/folder name/bar", &path));
643643
CheckDirs({});
644-
std::vector<std::pair<std::string, std::any>> options{{"some_option", 1}};
645-
EXPECT_RAISES_WITH_MESSAGE_THAT(NotImplemented,
646-
::testing::HasSubstr("options are not supported"),
647-
FileSystemFromUri("mock:///foo/bar", options, &path));
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));
648648
CheckDirs({});
649649
}
650650

‎cpp/src/arrow/filesystem/localfs_test.cc‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,13 +157,14 @@ TEST(FileSystemFromUri, LoadedRegisteredFactory) {
157157
EXPECT_EQ(fs->type_name(), "local");
158158

159159
// Validate extra options are forwarded to the factory.
160-
std::vector<std::pair<std::string, std::any>> options{
160+
FileSystemFactoryOptions options{
161161
{"example_option_string", std::string("example_value")},
162162
{"example_option_int", 42},
163163
{"example_typed_option",
164164
std::shared_ptr<ExampleTypedOption>(std::make_shared<ConcreteTypedOption>(12345))},
165165
};
166-
ASSERT_OK_AND_ASSIGN(fs, FileSystemFromUri("example:///hey/yo", options, &path));
166+
ASSERT_OK_AND_ASSIGN(fs,
167+
FileSystemFromUriAndOptions("example:///hey/yo", options, &path));
167168
EXPECT_EQ(path, "/hey/yo/example_value/42/12345");
168169
EXPECT_EQ(fs->type_name(), "local");
169170
}

‎cpp/src/arrow/filesystem/s3fs.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3603,8 +3603,7 @@ Result<std::string> ResolveS3BucketRegion(const std::string& bucket) {
36033603

36043604
auto kS3FileSystemModule = ARROW_REGISTER_FILESYSTEM(
36053605
"s3",
3606-
[](const arrow::util::Uri& uri,
3607-
const std::vector<std::pair<std::string, std::any>>& options,
3606+
[](const arrow::util::Uri& uri, const FileSystemFactoryOptions& options,
36083607
const io::IOContext& io_context,
36093608
std::string* out_path) -> Result<std::shared_ptr<fs::FileSystem>> {
36103609
if (!options.empty()) {

‎cpp/src/arrow/filesystem/s3fs_module_test.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,10 @@ TEST(S3Test, FromUri) {
8484
}
8585

8686
TEST(S3Test, FromUriRejectsOptions) {
87-
std::vector<std::pair<std::string, std::any>> options{{"some_option", 1}};
88-
EXPECT_RAISES_WITH_MESSAGE_THAT(NotImplemented,
89-
::testing::HasSubstr("options are not supported"),
90-
FileSystemFromUri("s3://bucket/key", options));
87+
FileSystemFactoryOptions options{{"some_option", 1}};
88+
EXPECT_RAISES_WITH_MESSAGE_THAT(
89+
NotImplemented, ::testing::HasSubstr("options are not supported"),
90+
FileSystemFromUriAndOptions("s3://bucket/key", options));
9191
}
9292

9393
} // namespace arrow::fs

‎cpp/src/arrow/testing/examplefs.cc‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,13 @@ namespace arrow::fs {
3030

3131
auto kExampleFileSystemModule = ARROW_REGISTER_FILESYSTEM(
3232
"example",
33-
[](const Uri& uri, const std::vector<std::pair<std::string, std::any>>& options,
33+
[](const Uri& uri, const FileSystemFactoryOptions& options,
3434
const io::IOContext& io_context,
3535
std::string* out_path) -> Result<std::shared_ptr<FileSystem>> {
3636
constexpr std::string_view kScheme = "example";
3737
EXPECT_EQ(uri.scheme(), kScheme);
3838
auto local_uri = "file" + uri.ToString().substr(kScheme.size());
39-
ARROW_ASSIGN_OR_RAISE(auto fs,
40-
FileSystemFromUri(local_uri, options, io_context, out_path));
39+
ARROW_ASSIGN_OR_RAISE(auto fs, FileSystemFromUri(local_uri, io_context, out_path));
4140
for (const auto& [key, value] : options) {
4241
EXPECT_TRUE(value.has_value());
4342
if (key == "example_option_string") {

0 commit comments

Comments
 (0)