Copilot commented on code in PR #50044:
URL: https://github.com/apache/arrow/pull/50044#discussion_r3372612776
##########
cpp/src/arrow/filesystem/filesystem.h:
##########
@@ -547,6 +572,30 @@ 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 only forwarded to schemes dispatched through a
+/// registered FileSystemFactory (currently "s3" and any scheme
+/// added via RegisterFileSystemFactory). They are ignored by the
+/// built-in schemes.
Review Comment:
The documentation says backend-specific `options` are "ignored by the
built-in schemes", but the implementation rejects non-empty options for schemes
that are not dispatched through a registered `FileSystemFactory` (e.g. `mock`
now returns NotImplemented). Please update the doc comment to match the actual
behavior (forwarded to registered factories; otherwise NotImplemented; and
factories without option support may ignore).
##########
cpp/src/arrow/filesystem/filesystem.h:
##########
@@ -563,6 +612,31 @@ 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 only forwarded to schemes dispatched through a
+/// registered FileSystemFactory (currently "s3" and any scheme
+/// added via RegisterFileSystemFactory). They are ignored by the
+/// built-in schemes.
Review Comment:
Same as above: this overload's doc comment claims options are "ignored by
the built-in schemes", but non-empty options are rejected for schemes without a
registered `FileSystemFactory`. Align the comment with current behavior so
callers know options may be ignored by some factories and otherwise will raise
NotImplemented.
--
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]
