WZhuo commented on code in PR #818:
URL: https://github.com/apache/iceberg-cpp/pull/818#discussion_r3526380969
##########
src/iceberg/arrow/arrow_io.cc:
##########
@@ -484,24 +485,63 @@ class ArrowOutputFile : public OutputFile {
} // namespace
Result<std::string> ArrowFileSystemFileIO::ResolvePath(const std::string&
file_location) {
- const auto pos = file_location.find("://");
- if (pos == std::string::npos) {
- return file_location;
- }
-
- auto path = arrow_fs_->PathFromUri(file_location);
+ // Detect whether the location is a URI by looking for a scheme component.
+ // A URI scheme (RFC 3986 ยง3.1) starts with a letter and is followed by any
+ // combination of letters, digits, '+', '-', or '.', ending at the first ':'.
+ // A single character before ':' is a Windows drive letter (e.g., "C:\..."),
+ // not a URI scheme.
+ auto colon_pos = file_location.find(':');
+ bool is_uri =
+ colon_pos != std::string::npos && colon_pos > 1 &&
+ std::isalpha(static_cast<unsigned char>(file_location[0])) &&
+ std::all_of(file_location.begin(),
+ file_location.begin() +
static_cast<std::ptrdiff_t>(colon_pos),
+ [](char c) {
+ return std::isalpha(static_cast<unsigned char>(c)) ||
+ std::isdigit(static_cast<unsigned char>(c)) || c ==
'+' ||
+ c == '-' || c == '.';
+ });
Review Comment:
The RFC 3986 URI scheme detection logic here (lines 493โ503) is duplicated
verbatim in `rest_file_io.cc` (`DetectBuiltinFileIO`, lines 59โ68). Consider
extracting a shared `bool IsUriScheme(std::string_view)` helper into e.g.
`iceberg/util/uri.h`. If the duplication is intentional to keep the
`rest_file_io` layer independent, a comment at each site noting the duplication
would suffice.
##########
src/iceberg/catalog/rest/rest_file_io.cc:
##########
@@ -51,12 +53,25 @@ std::unordered_map<std::string, std::string>
MergeFileIOProperties(
} // namespace
Result<BuiltinFileIOKind> DetectBuiltinFileIO(std::string_view location) {
- const auto pos = location.find("://");
- if (pos == std::string_view::npos) {
+ // Detect URI scheme using RFC 3986 rules: a scheme is 2+ chars starting with
+ // a letter, followed by letters/digits/+/-/., ending at ':'.
+ // A single char before ':' is a Windows drive letter, not a scheme.
+ auto colon_pos = location.find(':');
+ bool is_uri =
+ colon_pos != std::string_view::npos && colon_pos > 1 &&
+ std::isalpha(static_cast<unsigned char>(location[0])) &&
+ std::all_of(location.begin(),
+ location.begin() + static_cast<std::ptrdiff_t>(colon_pos),
[](char c) {
+ return std::isalpha(static_cast<unsigned char>(c)) ||
+ std::isdigit(static_cast<unsigned char>(c)) || c ==
'+' ||
+ c == '-' || c == '.';
+ });
Review Comment:
This RFC 3986 scheme detection is the same logic as in `arrow_io.cc`
(`ArrowFileSystemFileIO::ResolvePath`, lines 493โ503). See the comment there
about extracting a shared helper.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
