diff --git a/src/iceberg/arrow/arrow_io.cc b/src/iceberg/arrow/arrow_io.cc index 4c795badf..6831ede37 100644 --- a/src/iceberg/arrow/arrow_io.cc +++ b/src/iceberg/arrow/arrow_io.cc @@ -36,6 +36,7 @@ #include "iceberg/arrow/arrow_io_util.h" #include "iceberg/arrow/arrow_status_internal.h" #include "iceberg/util/macros.h" +#include "iceberg/util/uri.h" namespace iceberg::arrow { @@ -484,24 +485,51 @@ class ArrowOutputFile : public OutputFile { } // namespace Result 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)); + } + if (auto parsed = ::arrow::util::Uri::FromString(file_location); !parsed.ok()) { const auto& status = parsed.status(); return std::unexpected{ {.kind = ToErrorKind(status), .message = status.ToString()}}; } - std::string bucket_key = file_location.substr(pos + 3); + std::string bucket_key = file_location.substr(sep_pos + 3); bucket_key = bucket_key.substr(0, bucket_key.find_first_of("?#")); return ::arrow::util::UriUnescape(bucket_key); } diff --git a/src/iceberg/catalog/rest/rest_file_io.cc b/src/iceberg/catalog/rest/rest_file_io.cc index fe8a2b155..6fb2396aa 100644 --- a/src/iceberg/catalog/rest/rest_file_io.cc +++ b/src/iceberg/catalog/rest/rest_file_io.cc @@ -28,6 +28,7 @@ #include "iceberg/file_io.h" #include "iceberg/file_io_registry.h" #include "iceberg/util/macros.h" +#include "iceberg/util/uri.h" namespace iceberg::rest { @@ -51,12 +52,16 @@ std::unordered_map MergeFileIOProperties( } // namespace Result DetectBuiltinFileIO(std::string_view location) { - const auto pos = location.find("://"); - if (pos == std::string_view::npos) { + // Detect URI scheme using RFC 3986 rules. + // See iceberg/util/uri.h for the scheme grammar. + std::size_t colon_pos = 0; + bool is_uri = IsUriScheme(location, &colon_pos); + + if (!is_uri) { return BuiltinFileIOKind::kArrowLocal; } - const auto scheme = location.substr(0, pos); + const auto scheme = location.substr(0, colon_pos); if (scheme == "file") { return BuiltinFileIOKind::kArrowLocal; } diff --git a/src/iceberg/test/arrow_io_test.cc b/src/iceberg/test/arrow_io_test.cc index a30e2da93..fd554675a 100644 --- a/src/iceberg/test/arrow_io_test.cc +++ b/src/iceberg/test/arrow_io_test.cc @@ -444,6 +444,53 @@ TEST_F(LocalFileIOTest, PropagatesNonSchemeMismatchUriError) { EXPECT_THAT(read_res, HasErrorMessage("Cannot parse URI")); } +// Regression tests for #341: file: URIs with fewer than 3 slashes must be +// recognized as URIs and normalized before being passed to Arrow. +TEST_F(LocalFileIOTest, ResolvesFileUriSingleSlash) { + ASSERT_THAT(file_io_->WriteFile(temp_filepath_, "hello"), IsOk()); + // Build a cross-platform file:/path URI: normalize backslashes to forward slashes + // and ensure a leading '/' (Linux paths already have one; Windows drive letters don't). + std::string p = temp_filepath_; + std::ranges::replace(p, '\\', '/'); + std::string single_slash_uri = "file:" + (p.front() == '/' ? p : "/" + p); + auto read_res = file_io_->ReadFile(single_slash_uri, std::nullopt); + EXPECT_THAT(read_res, IsOk()); + EXPECT_THAT(read_res, HasValue(::testing::Eq("hello"))); +} + +TEST_F(LocalFileIOTest, ResolvesFileUriTripleSlash) { + ASSERT_THAT(file_io_->WriteFile(temp_filepath_, "hello"), IsOk()); + // Build a cross-platform file:///path URI: normalize backslashes to forward slashes + // and ensure exactly one leading '/' before the (possibly drive-lettered) path so + // that "file://" + "/" + path produces the canonical triple-slash form on all + // platforms. Linux: file:///tmp/... Windows: file:///C:/Users/... + std::string p = temp_filepath_; + std::ranges::replace(p, '\\', '/'); + std::string triple_slash_uri = "file://" + (p.front() == '/' ? p : "/" + p); + auto read_res = file_io_->ReadFile(triple_slash_uri, std::nullopt); + EXPECT_THAT(read_res, IsOk()); + EXPECT_THAT(read_res, HasValue(::testing::Eq("hello"))); +} + +TEST_F(LocalFileIOTest, FileUriWithAuthorityPassesThrough) { + // Non-regression: "file://host/path" is an authority-bearing URI and must NOT + // be rewritten to "file:///host/path" (which would change its meaning). + // Arrow's PathFromUri handles authority-bearing file URIs correctly. + auto read_res = file_io_->ReadFile("file://remotehost/share/file.avro", std::nullopt); + // The read will fail (no such host/path) but with an IO error from Arrow, + // NOT a mangled path like "/remotehost/share/file.avro" on the local filesystem. + EXPECT_THAT(read_res, IsError(ErrorKind::kIOError)); +} + +TEST_F(LocalFileIOTest, BareWindowsDriveLetterNotMisparsedAsUri) { + // Verify that C:/... is NOT treated as a URI scheme (single char before ':'). + // If it were misparsed as scheme "C", ReadFile would attempt URI resolution and + // fail with an Arrow URI error instead of a filesystem "not found" error. + auto read_res = file_io_->ReadFile("C:/Users/test/warehouse/file.avro", std::nullopt); + EXPECT_THAT(read_res, IsError(ErrorKind::kIOError)); + EXPECT_THAT(read_res, HasErrorMessage("Failed to open local file")); +} + TEST_F(LocalFileIOTest, FallbackDecodesPercentEncodingInKey) { std::string decoded_path = temp_filepath_ + " x"; ASSERT_THAT(file_io_->WriteFile(decoded_path, "raw"), IsOk()); diff --git a/src/iceberg/test/rest_file_io_test.cc b/src/iceberg/test/rest_file_io_test.cc index 7f41b0b46..d10f6ac9b 100644 --- a/src/iceberg/test/rest_file_io_test.cc +++ b/src/iceberg/test/rest_file_io_test.cc @@ -82,6 +82,17 @@ TEST(RestFileIOTest, DetectBuiltinKindFromScheme) { HasValue(::testing::Eq(BuiltinFileIOKind::kArrowLocal))); } +// Regression test for #341: file: URIs with fewer than 3 slashes +TEST(RestFileIOTest, DetectBuiltinKindFileUriSingleSlash) { + EXPECT_THAT(DetectBuiltinFileIO("file:/tmp/warehouse"), + HasValue(::testing::Eq(BuiltinFileIOKind::kArrowLocal))); + EXPECT_THAT(DetectBuiltinFileIO("file:/C:/iceberg_warehouse/db"), + HasValue(::testing::Eq(BuiltinFileIOKind::kArrowLocal))); + // Windows drive letter (single char before ':') โ†’ local path, not a URI + EXPECT_THAT(DetectBuiltinFileIO("C:/iceberg_warehouse/db"), + HasValue(::testing::Eq(BuiltinFileIOKind::kArrowLocal))); +} + TEST(RestFileIOTest, DetectBuiltinKindRejectsUnsupportedScheme) { auto result = DetectBuiltinFileIO("gs://bucket/warehouse"); EXPECT_THAT(result, IsError(ErrorKind::kNotSupported)); diff --git a/src/iceberg/util/uri.h b/src/iceberg/util/uri.h new file mode 100644 index 000000000..6719e9519 --- /dev/null +++ b/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 +#include +#include +#include + +/// \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(value[0]))) { + return false; + } + bool valid = std::ranges::all_of(value.substr(1, colon_pos - 1), [](char c) { + return std::isalpha(static_cast(c)) || + std::isdigit(static_cast(c)) || c == '+' || c == '-' || + c == '.'; + }); + if (valid && scheme_colon_pos != nullptr) { + *scheme_colon_pos = colon_pos; + } + return valid; +} + +} // namespace iceberg