Repository navigation
Conversation
Add LocationUtil::HasScheme, ResolveLocation and RelativizeLocation for v4 relative paths, as defined in the spec's "Paths in Metadata" section. - HasScheme is strict RFC 3986 scheme detection on every platform. - ResolveLocation returns absolute paths unchanged and joins relative paths to the table location with "/", with no normalization or URI parsing. If the table location already ends in "/", no second separator is added, since object stores such as S3 treat "//" as part of the key. - RelativizeLocation strips the table location and one separator when a path is under it, and otherwise returns the path unchanged. Tests cover every example in the spec's path resolution table, plus trailing slashes, sibling prefixes, schemes, escaped characters and round trips. Closes apache#1123
c6a7e77 to
4d2ac7a
Compare
| // one the writer stored. Both parts are opaque strings in whatever encoding | ||
| // the writer used, and they are copied byte for byte. | ||
| // | ||
| // The spec recommends a table location that does not end in `/`, because |
There was a problem hiding this comment.
I'm not sure this is the right solution, but from the spec I can see the trailing / is kept, I tend to align with spec[1], correct me if I'm wrong :-)
There was a problem hiding this comment.
Quote comment from java:
* The separator is appended unconditionally; {@code tableLocation} is expected
* not to end with {@code /} and {@code location} is expected not to start
* with {@code /}. Otherwise the result will contain a duplicate {@code //}.
There was a problem hiding this comment.
The spec says exactly this:
The recommended convention for table location is to not end in a path separator because the join process would add a second separator character. (See example below.)
https://iceberg.apache.org/spec/#path-resolution
So it recommend not to include the trailing / for table location, but this diff go ahead and enforce that. It's very problematic for S3 if you include double back slash as they are not treated as file path. So s3://foo//bar is actually different then s3://foo/bar. Super confusing and error prone. That's why i just enforce that in the diff.
We could also add an assertion but I feel it's a bit too strong and there's an easy remedy.
|
|
||
| /// \brief Whether a location starts with a URI scheme. | ||
| /// | ||
| /// This is the spec's test for an absolute path (see "Paths in Metadata"): |
There was a problem hiding this comment.
Should we copy the descriptions from java impl?
There was a problem hiding this comment.
sure. but the difference needs to be called out. So will make this change after previous thread is resolved.
|
What is |
A local script that agent referenced. I've fixed that. |
Closes #1123.
What
Adds three helpers to
LocationUtil(src/iceberg/util/location_util.{h,cc}) for v4 relative paths, as defined in the spec's Paths in Metadata section (apache/iceberg#15630):HasScheme(location): true when the text before the first:is a non-empty RFC 3986 scheme.ResolveLocation(table_location, location) -> std::string: absolute paths are returned unchanged; relative paths becometable_location + "/" + location.RelativizeLocation(table_location, location) -> std::string_view: whenlocationstarts withtable_locationimmediately followed by/, returns the rest after that separator; otherwise returnslocationunchanged.Nothing calls them yet. Using them in metadata, manifests and catalogs is left to follow-up PRs.
Trailing separator on the table location
The spec recommends that a table location not end in
/, because the join would otherwise produce//(its "Duplicate separator" example). This PR applies that recommendation more strongly: when the table location ends in/,ResolveLocationdoesn't add a second separator (s3://bucket/db/table/+data/00000-0.parquet→s3://bucket/db/table/data/00000-0.parquet). Object stores such as S3 treat//as part of the key and advise against it.Comparison with Java
Matches Java's
LocationUtil.hasScheme/resolveLocation/relativizeLocation(apache/iceberg#16174, apache/iceberg#17807), with these differences:/; this PR handles the trailing/as described above.RelativizeLocationreturns astd::string_viewinto itslocationargument, because the result is always a suffix of that argument and copying it isn't needed.Edge cases that match Java: only one separator is stripped, so
<loc>//xrelativizes to/xand resolves back to<loc>//x, and the table location itself is kept absolute.Not covered: adding a scheme to pre-v4 paths
The spec's Path Resolution section ends with a table of examples. Its last row, "Missing scheme", is about v3 and earlier tables: a stored path without a scheme, such as
/wh/db/table/data/00000-0.parquet, should get one added (hdfs://wh/db/table/data/00000-0.parquetin the example). That path is already complete, so this isn't relative resolution, and the spec doesn't say where the scheme comes from. This PR doesn't do it; that is follow-up work. For that row, the test only checks that such a path isn't treated as absolute (HasSchemeis false). The other rows are tested throughResolveLocation.Tests
src/iceberg/test/location_util_test.cc:HasSchemegrammar cases: leading digit or+,_,/before the colon, empty input, the Windows extended-length form, drive letters../.., plus both kinds of duplicate separator./tvs/t2), the table location itself, different and case-differing schemes, trailing slashes./on the table location.Built with CMake/Ninja (gcc-14, Debug); ctest --test-dir build passes (21/21) and pre-commit is clean.