Copilot commented on code in PR #50044:
URL: https://github.com/apache/arrow/pull/50044#discussion_r3381081541
##########
cpp/src/arrow/testing/examplefs.cc:
##########
@@ -15,9 +15,13 @@
// specific language governing permissions and limitations
// under the License.
+#include <any>
+#include <typeinfo>
Review Comment:
`#include <typeinfo>` is unused in this translation unit and can trigger
-Wunused-include / IWYU warnings in some builds.
##########
cpp/src/arrow/filesystem/localfs_test.cc:
##########
@@ -144,10 +152,21 @@ TEST(FileSystemFromUri, LoadedRegisteredFactory) {
EXPECT_THAT(FileSystemFromUri("example:///hey/yo", &path),
Raises(StatusCode::Invalid));
EXPECT_THAT(LoadFileSystemFactories(ARROW_FILESYSTEM_EXAMPLE_LIBPATH), Ok());
-
ASSERT_OK_AND_ASSIGN(auto fs, FileSystemFromUri("example:///hey/yo", &path));
EXPECT_EQ(path, "/hey/yo");
EXPECT_EQ(fs->type_name(), "local");
+
+ // Validate extra options are forwarded to the factory.
+ FileSystemFactoryOptions options{
+ {"example_option_string", std::string("example_value")},
+ {"example_option_int", 42},
+ {"example_typed_option",
+
std::shared_ptr<ExampleTypedOption>(std::make_shared<ConcreteTypedOption>(12345))},
+ };
+ ASSERT_OK_AND_ASSIGN(fs,
+ FileSystemFromUriAndOptions("example:///hey/yo",
options, &path));
+ EXPECT_EQ(path, "/hey/yo/example_value/42/12345");
+ EXPECT_EQ(fs->type_name(), "local");
Review Comment:
The legacy-factory compatibility path (registering a 3-arg factory and
calling FileSystemFromUriAndOptions with non-empty options) is an important
behavior change, but it isn't exercised here. Adding a small assertion that a
legacy registered factory (e.g. the already-registered "slowfile" factory)
rejects non-empty options would prevent regressions.
##########
cpp/src/arrow/filesystem/filesystem.h:
##########
@@ -357,13 +359,43 @@ 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(
+ "This filesystem does not support additional options");
+ }
+ return fn(uri, ctx, out_path);
Review Comment:
The compatibility wrapper for legacy (3-arg) factories returns a very
generic NotImplemented message without the scheme or how many options were
provided. Including the scheme and count makes debugging much easier and aligns
with the more detailed error used elsewhere in FileSystemFromUriAndOptions.
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -3603,11 +3603,17 @@ Result<std::string> ResolveS3BucketRegion(const
std::string& bucket) {
auto kS3FileSystemModule = ARROW_REGISTER_FILESYSTEM(
"s3",
- [](const arrow::util::Uri& uri, const io::IOContext& io_context,
+ [](const arrow::util::Uri& uri, const FileSystemFactoryOptions& options,
+ const io::IOContext& io_context,
std::string* out_path) -> Result<std::shared_ptr<fs::FileSystem>> {
+ if (!options.empty()) {
+ return Status::NotImplemented(
+ "S3 filesystem factory options are not supported yet, got: ",
options.size(),
+ " options");
Review Comment:
The NotImplemented message here always says "X options" even when a single
option is provided. Using "option(s)" (and dropping the extra colon after
"got") keeps the wording consistent with other option-related errors in
filesystem.cc.
--
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]
