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 4 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.
3 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.