Copilot commented on code in PR #50044:
URL: https://github.com/apache/arrow/pull/50044#discussion_r3380875604
##########
cpp/src/arrow/testing/examplefs.cc:
##########
@@ -26,12 +30,39 @@ 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") {
+ EXPECT_TRUE(value.type() == typeid(std::string));
+ if (out_path != nullptr) {
+ *out_path += "/" + std::any_cast<std::string>(value);
+ }
+ } else if (key == "example_option_int") {
+ EXPECT_TRUE(value.type() == typeid(int));
+ if (out_path != nullptr) {
+ *out_path += "/" + std::to_string(std::any_cast<int>(value));
+ }
+ } else if (key == "example_typed_option") {
Review Comment:
`EXPECT_TRUE(value.type() == ...)` is non-fatal, but the subsequent
`std::any_cast<T>(value)` will throw `std::bad_any_cast` on mismatch and can
abort the test/module. Use the pointer form of `std::any_cast` (or `ASSERT_*`)
to avoid throwing and to report a clean test failure.
##########
cpp/src/arrow/filesystem/filesystem.h:
##########
@@ -563,6 +620,32 @@ Result<std::shared_ptr<FileSystem>>
FileSystemFromUri(const std::string& uri,
const io::IOContext&
io_context,
std::string* out_path =
NULLPTR);
+/// \brief Create a new FileSystem by URI with a custom IO context with
backend-specific
+/// filesystem options
+///
+/// Recognized schemes are "file", "mock", "hdfs", "viewfs", "s3",
+/// "gs" and "gcs".
+///
+/// Support for other schemes can be added using RegisterFileSystemFactory.
+///
+/// \param[in] uri a URI-based path, ex: file:///some/local/path
+/// \param[in] options a list of backend-specific filesystem options
+/// Each option is a (name, value) pair.
+/// The expected type is specific to the backend and
+/// option name.
+/// Options are forwarded to schemes dispatched through a registered
+/// FileSystemFactory. A registered factory that does not support
options
+/// (currently "s3") returns NotImplemented for non-empty options.
+/// Schemes not handled by a registered factory will also return
+/// NotImplemented for non-empty options.
Review Comment:
This overload’s docstring also states only "s3" currently rejects options.
In practice any registered factory using the legacy 3-arg adapter (including
built-in "file"/"local") will return NotImplemented for non-empty options, so
the comment should describe the behavior generically (same as the other
overload).
##########
cpp/src/arrow/filesystem/filesystem.h:
##########
@@ -547,6 +579,31 @@ ARROW_EXPORT
Result<std::shared_ptr<FileSystem>> FileSystemFromUri(const std::string& uri,
std::string* out_path =
NULLPTR);
+/// \brief Create a new FileSystem by URI with extended backend-specific
filesystem
+/// options
+///
+/// Recognized schemes are "file", "mock", "hdfs", "viewfs", "s3",
+/// "gs" and "gcs".
+///
+/// Support for other schemes can be added using RegisterFileSystemFactory.
+///
+/// \param[in] uri the URI to give access to
+/// \param[in] options a list of backend-specific filesystem options
+/// Each option is a (name, value) pair.
+/// The expected type is specific to the backend and
+/// option name.
+/// Options are forwarded to schemes dispatched through a registered
+/// FileSystemFactory. A registered factory that does not support
options
+/// (currently "s3") returns NotImplemented for non-empty options.
+/// Schemes not handled by a registered factory will also return
+/// NotImplemented for non-empty options.
Review Comment:
The docstring claims only the "s3" registered factory currently rejects
options, but other registered schemes using the legacy 3-arg factory adapter
(e.g. "file"/"local") will also return NotImplemented for non-empty options.
The comment should describe the behavior generically to avoid misleading API
users.
--
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]
