wgtmac commented on code in PR #818:
URL: https://github.com/apache/iceberg-cpp/pull/818#discussion_r3831162949
##########
src/iceberg/arrow/arrow_io.cc:
##########
@@ -484,24 +485,51 @@ 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;
+ // Detect whether the location is a URI by looking for a scheme component.
+ // See iceberg/util/uri.h for the RFC 3986 scheme grammar.
+ std::size_t colon_pos = 0;
+ bool is_uri = IsUriScheme(file_location, &colon_pos);
+
+ if (!is_uri) {
+ return file_location; // Bare local path (Unix or Windows drive letter)
+ }
+
+ // Normalize authority-less file: URI short forms to the canonical
three-slash
+ // form so that Arrow's PathFromUri can parse them. Java Iceberg may write
+ // "file:/path" (one slash) which lacks the "://" that Arrow expects.
+ // Authority-bearing URIs like "file://host/path" (char after "file://" is
not
+ // '/') are already valid RFC 3986 and pass through unchanged.
+ std::string normalized = file_location;
+ if (normalized.starts_with("file:/") && !normalized.starts_with("file://")) {
+ // file:/path → file:///path (single-slash shorthand, no authority)
+ normalized = "file:///" + normalized.substr(6);
}
- auto path = arrow_fs_->PathFromUri(file_location);
+ auto path = arrow_fs_->PathFromUri(normalized);
if (path.ok()) {
return std::move(path).ValueOrDie();
}
// Foreign alias (s3a/s3n): validate via Arrow's parser, then percent-decode
the
// scheme-less key (substring keeps a Windows drive letter's ':' that host()
drops).
+ const auto sep_pos = file_location.find("://");
+ if (sep_pos == std::string::npos) {
+ // URI without "://" that is not file: — attempt best-effort strip of
scheme
+ auto scheme_end = colon_pos + 1;
+ while (scheme_end < file_location.size() && file_location[scheme_end] ==
'/') {
+ ++scheme_end;
+ }
+ // Keep one leading slash for absolute paths
+ return std::string(
+ file_location.substr(scheme_end > colon_pos + 1 ? scheme_end - 1 :
scheme_end));
Review Comment:
This fallback is too broad. `file:relative/path` and `s3:/bucket/key` are
stripped to local paths after `PathFromUri` fails, so a URI can be read from
the wrong local file. Please keep this fallback only for supported foreign
aliases with `://` and return the original parse error otherwise.
##########
src/iceberg/util/uri.h:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include <algorithm>
+#include <cctype>
+#include <cstddef>
+#include <string_view>
+
+/// \file iceberg/util/uri.h
+/// \brief URI scheme detection utilities per RFC 3986.
+
+namespace iceberg {
+
+/// \brief Check whether a string begins with a valid RFC 3986 URI scheme
+/// followed by ':'.
+///
+/// A scheme (RFC 3986 §3.1) is defined as:
+/// scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
+///
+/// A single character before ':' is treated as a Windows drive letter, not a
+/// scheme (e.g., "C:\path").
+///
+/// \param value The string to inspect.
+/// \param scheme_colon_pos If non-null and the function returns true, set to
the
+/// position of the scheme-delimiting ':' so callers can reuse it without a
+/// redundant search.
+/// \return true if \p value starts with a valid URI scheme followed by ':'.
+inline bool IsUriScheme(std::string_view value, std::size_t* scheme_colon_pos
= nullptr) {
+ auto colon_pos = value.find(':');
+ // Reject if there is no ':', an empty scheme (colon_pos == 0), or only a
+ // single character before ':' (colon_pos == 1), which is a Windows drive
+ // letter (e.g. "C:\path"), not a URI scheme.
+ if (colon_pos == std::string_view::npos || colon_pos <= 1) {
Review Comment:
`a:` is a valid RFC 3986 scheme and Java accepts it. Please move the
Windows-drive exception out of this helper and only treat `C:/` or `C:\\` as
paths at the call sites; otherwise custom one-character schemes are rejected.
##########
src/iceberg/catalog/rest/rest_file_io.cc:
##########
@@ -51,12 +52,16 @@ std::unordered_map<std::string, std::string>
MergeFileIOProperties(
} // namespace
Result<BuiltinFileIOKind> DetectBuiltinFileIO(std::string_view location) {
Review Comment:
This code no longer matches the current FileIO resolution flow. Please
rebase this change onto the current resolver design and keep the Arrow `file:/`
fix separately.
##########
src/iceberg/arrow/arrow_io.cc:
##########
@@ -484,24 +485,51 @@ 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;
+ // Detect whether the location is a URI by looking for a scheme component.
+ // See iceberg/util/uri.h for the RFC 3986 scheme grammar.
+ std::size_t colon_pos = 0;
+ bool is_uri = IsUriScheme(file_location, &colon_pos);
+
+ if (!is_uri) {
+ return file_location; // Bare local path (Unix or Windows drive letter)
+ }
+
+ // Normalize authority-less file: URI short forms to the canonical
three-slash
+ // form so that Arrow's PathFromUri can parse them. Java Iceberg may write
+ // "file:/path" (one slash) which lacks the "://" that Arrow expects.
+ // Authority-bearing URIs like "file://host/path" (char after "file://" is
not
+ // '/') are already valid RFC 3986 and pass through unchanged.
+ std::string normalized = file_location;
+ if (normalized.starts_with("file:/") && !normalized.starts_with("file://")) {
Review Comment:
URI schemes are case-insensitive. Please normalize the scheme before
checking `file:/`; otherwise `FILE:/...` can fall through and be treated as a
local path.
##########
src/iceberg/util/uri.h:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include <algorithm>
+#include <cctype>
+#include <cstddef>
+#include <string_view>
+
+/// \file iceberg/util/uri.h
+/// \brief URI scheme detection utilities per RFC 3986.
+
+namespace iceberg {
+
+/// \brief Check whether a string begins with a valid RFC 3986 URI scheme
+/// followed by ':'.
+///
+/// A scheme (RFC 3986 §3.1) is defined as:
+/// scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
+///
+/// A single character before ':' is treated as a Windows drive letter, not a
+/// scheme (e.g., "C:\path").
+///
+/// \param value The string to inspect.
+/// \param scheme_colon_pos If non-null and the function returns true, set to
the
+/// position of the scheme-delimiting ':' so callers can reuse it without a
+/// redundant search.
+/// \return true if \p value starts with a valid URI scheme followed by ':'.
+inline bool IsUriScheme(std::string_view value, std::size_t* scheme_colon_pos
= nullptr) {
+ auto colon_pos = value.find(':');
+ // Reject if there is no ':', an empty scheme (colon_pos == 0), or only a
+ // single character before ':' (colon_pos == 1), which is a Windows drive
+ // letter (e.g. "C:\path"), not a URI scheme.
+ if (colon_pos == std::string_view::npos || colon_pos <= 1) {
+ return false;
+ }
+ if (!std::isalpha(static_cast<unsigned char>(value[0]))) {
Review Comment:
Use explicit ASCII predicates instead of `std::isalpha` and `std::isdigit`.
RFC 3986 defines ALPHA and DIGIT as ASCII, while these ctype calls are
locale-sensitive.
--
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]
