Copilot commented on code in PR #50044:
URL: https://github.com/apache/arrow/pull/50044#discussion_r3402021049
##########
cpp/src/arrow/filesystem/filesystem.h:
##########
@@ -357,13 +359,44 @@ class ARROW_EXPORT FileSystem
bool default_async_is_sync_ = true;
};
+using FileSystemFactoryOptions = std::vector<std::pair<std::string, std::any>>;
+
struct FileSystemFactory {
std::function<Result<std::shared_ptr<FileSystem>>(
- const Uri& uri, const io::IOContext& io_context, std::string* out_path)>
+ const Uri& uri, const FileSystemFactoryOptions& options,
+ const io::IOContext& io_context, std::string* out_path)>
function;
std::string_view file;
int line;
+ /// Construct from an options-aware factory function.
+ FileSystemFactory(std::function<Result<std::shared_ptr<FileSystem>>(
+ const Uri&, const FileSystemFactoryOptions&, const
io::IOContext&,
+ std::string*)>
+ fn,
+ std::string_view file, int line)
+ : function(std::move(fn)), file(file), line(line) {}
+
+ /// Construct from a non-options aware factory function maintaining source
compatibility
+ /// with existing factories.
+ FileSystemFactory(std::function<Result<std::shared_ptr<FileSystem>>(
+ const Uri&, const io::IOContext&, std::string*)>
+ fn,
+ std::string_view file, int line)
+ : function([fn = std::move(fn)](
+ const Uri& uri, const FileSystemFactoryOptions& options,
+ const io::IOContext& ctx,
+ std::string* out_path) ->
Result<std::shared_ptr<FileSystem>> {
+ if (!options.empty()) {
+ return Status::NotImplemented(
+ "Filesystem factory does not support additional options, got ",
+ options.size(), " option(s)");
+ }
+ return fn(uri, ctx, out_path);
Review Comment:
The legacy (3-arg) factory wrapper calls `fn(...)` unconditionally. If a
caller accidentally registers an empty/null `std::function` (or passes nullptr
through `ARROW_REGISTER_FILESYSTEM`), this will throw `std::bad_function_call`
and likely terminate rather than returning an Arrow `Status`. Consider guarding
and returning a clear `Status::Invalid` instead.
##########
cpp/src/arrow/testing/examplefs.cc:
##########
@@ -26,12 +29,41 @@ namespace arrow::fs {
auto kExampleFileSystemModule = ARROW_REGISTER_FILESYSTEM(
"example",
- [](const Uri& uri, const io::IOContext& io_context,
+ [](const Uri& uri, const FileSystemFactoryOptions& options,
+ const io::IOContext& io_context,
std::string* out_path) -> Result<std::shared_ptr<FileSystem>> {
constexpr std::string_view kScheme = "example";
EXPECT_EQ(uri.scheme(), kScheme);
auto local_uri = "file" + uri.ToString().substr(kScheme.size());
- return FileSystemFromUri(local_uri, io_context, out_path);
+ ARROW_ASSIGN_OR_RAISE(auto fs, FileSystemFromUri(local_uri, io_context,
out_path));
+ for (const auto& [key, value] : options) {
+ EXPECT_TRUE(value.has_value());
+ if (key == "example_option_string") {
+ if (const auto* s = std::any_cast<std::string>(&value)) {
+ if (out_path != nullptr) *out_path += "/" + *s;
+ } else {
+ ADD_FAILURE() << "example_option_string has wrong type";
+ }
+ } else if (key == "example_option_int") {
+ if (const auto* i = std::any_cast<int>(&value)) {
+ if (out_path != nullptr) *out_path += "/" + std::to_string(*i);
+ } else {
+ ADD_FAILURE() << "example_option_int has wrong type";
+ }
+ } else if (key == "example_typed_option") {
+ if (const auto* opt =
+ std::any_cast<std::shared_ptr<ExampleTypedOption>>(&value)) {
+ if (out_path != nullptr) {
+ *out_path += "/" + std::to_string((*opt)->value());
+ }
+ } else if (out_path != nullptr) {
+ *out_path += "/typed_cast_failed";
+ }
Review Comment:
For `example_typed_option`, a type mismatch is only reflected by mutating
`out_path` when it is non-null; if `out_path` is null the failure is silently
ignored (unlike the other option branches which call `ADD_FAILURE()`). It would
be more reliable to always record a test failure on a bad cast.
--
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]
