fix: correct URI-vs-path handling in PlanFiles() for Java-written file: paths - #818
fix: correct URI-vs-path handling in PlanFiles() for Java-written file: paths#818yadavay-amzn wants to merge 5 commits into
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
| 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 == '.'; | ||
| }); |
There was a problem hiding this comment.
The RFC 3986 URI scheme detection logic here is duplicated verbatim in rest_file_io.cc DetectBuiltinFileIO. 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.
There was a problem hiding this comment.
Good call - extracted the RFC 3986 scheme detection into a shared header-only helper IsUriScheme(std::string_view) in src/iceberg/util/uri.h, and both ResolvePath() here and DetectBuiltinFileIO() in rest_file_io.cc now call it, so the logic is no longer duplicated.
| 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 == '.'; | ||
| }); |
There was a problem hiding this comment.
This RFC 3986 scheme detection is the same logic as in arrow_io.cc ArrowFileSystemFileIO::ResolvePath. See the comment there about extracting a shared helper.
There was a problem hiding this comment.
Done - this and the arrow_io.cc site now both call the shared IsUriScheme() helper in src/iceberg/util/uri.h; the duplicated detection block is removed.
| return file_location; // Bare local path (Unix or Windows drive letter) | ||
| } | ||
|
|
||
| auto colon_pos = file_location.find(':'); |
There was a problem hiding this comment.
Now we have two calls to find. That's probably acceptable, but we could also add an output parameter to return colon_pos. Not a strong opinion.
There was a problem hiding this comment.
Done - IsUriScheme now takes an optional std::size_t* scheme_colon_pos out-param, and both ResolvePath here and DetectBuiltinFileIO in rest_file_io.cc reuse it instead of calling find(':') again.
| } | ||
| // Keep one leading slash for absolute paths | ||
| return std::string( | ||
| file_location.substr(scheme_end > colon_pos + 1 ? scheme_end - 1 : scheme_end)); |
There was a problem hiding this 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.
| // 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) { |
There was a problem hiding this 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.
| if (colon_pos == std::string_view::npos || colon_pos <= 1) { | ||
| return false; | ||
| } | ||
| if (!std::isalpha(static_cast<unsigned char>(value[0]))) { |
There was a problem hiding this 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.
| // 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://")) { |
There was a problem hiding this 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.
| @@ -51,12 +52,16 @@ std::unordered_map<std::string, std::string> MergeFileIOProperties( | |||
| } // namespace | |||
|
|
|||
| Result<BuiltinFileIOKind> DetectBuiltinFileIO(std::string_view location) { | |||
There was a problem hiding this 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.
4 participants
What
Fixes #341.
PlanFiles()(viaResolvePath()/DetectBuiltinFileIO()) mis-handledfile:locations written by Java Iceberg in the short formfile:/C:/warehouse/...(fewer than three slashes): the value was not recognized as a URI and failed to resolve, so planning broke on Windows-style paths.How
:) instead of searching for://.file:forms (file:/path,file:/C:/...) to the canonicalfile:///form before handing them to Arrow'sPathFromUri.file://host/path,s3://bucket/...) and bare local paths pass through unchanged.Tests
Regression tests added in
arrow_io_test.ccandrest_file_io_test.cccovering the Javafile:/C:/...form, the canonicalfile:///form, andfile://host/pathpass-through. Build 788/788, tests 18/18 green.This contribution was authored with assistance from Claude Opus 4.8, in line with the Iceberg guidelines for AI-assisted contributions (https://iceberg.apache.org/contribute/#guidelines-for-ai-assisted-contributions). All changes were reviewed and tested by the author.