fix: correct URI-vs-path handling in PlanFiles() for Java-written file: paths - #818

Open

yadavay-amzn wants to merge 5 commits into

apache:main from

yadavay-amzn:fix-341-planfiles-path

Open

fix: correct URI-vs-path handling in PlanFiles() for Java-written file: paths #818
yadavay-amzn wants to merge 5 commits into
apache:main from
yadavay-amzn:fix-341-planfiles-path

Conversation

@yadavay-amzn

@yadavay-amzn yadavay-amzn commented Jul 6, 2026

Copy link

Copy Markdown

Contributor

What

Fixes #341. PlanFiles() (via ResolvePath() / DetectBuiltinFileIO()) mis-handled file: locations written by Java Iceberg in the short form file:/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

Detect the URI scheme per RFC 3986 (a valid scheme token followed by :) instead of searching for ://.
Normalize authority-less file: forms (file:/path, file:/C:/...) to the canonical file:/// form before handing them to Arrow's PathFromUri.
Authority-bearing URIs (file://host/path, s3://bucket/...) and bare local paths pass through unchanged.

Tests

Regression tests added in arrow_io_test.cc and rest_file_io_test.cc covering the Java file:/C:/... form, the canonical file:/// form, and file://host/path pass-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.

yadavay-amzn added 2 commits

July 6, 2026 02:17

@yadavay-amzn


 Fix URI-vs-path handling in PlanFiles() for local/Windows paths ( apac...

e485afa

...he#341)

@yadavay-amzn


 Make file-URI tests cross-platform (fix Windows CI) ( apache#341 )

d30d697

WZhuo

This comment was marked as duplicate.

WZhuo

WZhuo reviewed

Jul 6, 2026

View reviewed changes

src/iceberg/arrow/arrow_io.cc Outdated

Comment on lines +493 to +503

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 == '.';

});

@WZhuo WZhuo Jul 6, 2026 •

edited

Loading

Copy link

Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@yadavay-amzn yadavay-amzn Jul 6, 2026

Copy link

Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

WZhuo

WZhuo reviewed

Jul 6, 2026

View reviewed changes

src/iceberg/catalog/rest/rest_file_io.cc Outdated

Comment on lines +59 to +68

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 == '.';

});

@WZhuo WZhuo Jul 6, 2026 •

edited

Loading

Copy link

Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@yadavay-amzn yadavay-amzn Jul 6, 2026

Copy link

Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

yadavay-amzn added 2 commits

July 6, 2026 05:30

@yadavay-amzn


 Extract shared IsUriScheme helper; use std::ranges::replace ( apache#341 )

e169f81

@yadavay-amzn


 Clarify colon_pos check in IsUriScheme ( apache#341 )

9d4858c

zhjwpku

zhjwpku reviewed

Jul 6, 2026

View reviewed changes

src/iceberg/arrow/arrow_io.cc Outdated

return file_location; // Bare local path (Unix or Windows drive letter)

}

auto colon_pos = file_location.find(':');

@zhjwpku zhjwpku Jul 6, 2026

Copy link

Copy Markdown

Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@yadavay-amzn yadavay-amzn Jul 6, 2026

Copy link

Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@yadavay-amzn


 Reuse scheme colon position via IsUriScheme out-param ( apache#341 )

14d63b8

wgtmac

wgtmac requested changes

Aug 21, 2026

View reviewed changes

src/iceberg/arrow/arrow_io.cc

}

// Keep one leading slash for absolute paths

return std::string(

file_location.substr(scheme_end > colon_pos + 1 ? scheme_end - 1 : scheme_end));

@wgtmac wgtmac Aug 21, 2026

Copy link

Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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) {

@wgtmac wgtmac Aug 21, 2026

Copy link

Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/util/uri.h

if (colon_pos == std::string_view::npos || colon_pos <= 1) {

return false;

}

if (!std::isalpha(static_cast<unsigned char>(value[0]))) {

@wgtmac wgtmac Aug 21, 2026

Copy link

Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/iceberg/arrow/arrow_io.cc

// 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://")) {

@wgtmac wgtmac Aug 21, 2026

Copy link

Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/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) {

@wgtmac wgtmac Aug 21, 2026

Copy link

Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Labels

None yet

4 participants

@yadavay-amzn @wgtmac @WZhuo @zhjwpku

Uh oh!

Conversation

@yadavay-amzn yadavay-amzn commented Jul 6, 2026

What

How

Tests

Uh oh!

This comment was marked as duplicate.

Uh oh!

@WZhuo WZhuo Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@yadavay-amzn yadavay-amzn Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

@WZhuo WZhuo Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@yadavay-amzn yadavay-amzn Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

@zhjwpku zhjwpku Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

@yadavay-amzn yadavay-amzn Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

@wgtmac wgtmac Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

@wgtmac wgtmac Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

@wgtmac wgtmac Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

@wgtmac wgtmac Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

@wgtmac wgtmac Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

@WZhuo WZhuo Jul 6, 2026 •

edited

Loading

@WZhuo WZhuo Jul 6, 2026 •

edited

Loading