Core: Resolve relative paths in V4 manifest reader - #17434
Conversation
| // Package-private only so the manifest reader can store the location resolved against the | ||
| // table location; other callers must go through construction. | ||
| void setLocation(String newLocation) { | ||
| this.location = newLocation; | ||
| } | ||
|
|
There was a problem hiding this comment.
An alternative to exposing the package private setLocation method is doing it through set with ordinal. But that would mean computing location's projected index from the read schema at resolution time and handling the not-projected case, for both the entry and its nested DV.
The package-private setLocation writes the field directly and bypasses the projection layer, so it's correct and fast regardless of what's projected.
V4 manifests may store file locations relative to the table location. Resolve relative data file, deletion vector, and leaf manifest locations against the table location when reading, so callers always see absolute paths. The table location is required (per the v4 spec, relative paths must be resolved before use); absolute paths pass through unchanged.
e5bf7bf to
b190b35
Compare
| static Builder builder(InputFile file, Map<Integer, PartitionSpec> specsById) { | ||
| return new Builder(file, specsById); | ||
| static Builder builder( | ||
| InputFile file, Map<Integer, PartitionSpec> specsById, String tableLocation) { |
There was a problem hiding this comment.
I think making this required is correct since we either have it from the existing metadata or it needs to be provided (e.g. by catalog). I don't see a way of knowing wheater paths are absolute, so this seems reasonable.
| private static final TrackedFile FILE_B = dataFile("data-b.parquet", partition(2)); | ||
| private static final TrackedFile EQ_DELETES_A = deleteFile("eq-deletes-a.parquet", partition(1)); | ||
| private static final TrackedFile EQ_DELETES_B = deleteFile("eq-deletes-b.parquet", partition(2)); | ||
| private static final TrackedFile FILE_A = dataFile("s3://bucket/data-a.parquet", partition(1)); |
There was a problem hiding this comment.
I don't think this is the right way to update the tests. The default going forward will be relative and it appears we switched the datafiles to absolute, but now they don't reflect the default expected behavior.
I think it makes more sense to update where we're validating locations to compare the resolved location rather than changing the content of the manifests.
There was a problem hiding this comment.
I did this way because the reader produces resolved URLs, so it probably made sense to compare against the contract of expecting absolute URLs. I have reverted this and added a small resolved() test utility method and used it n the assertions.
|
|
||
| @ParameterizedTest | ||
| @FieldSource("MANIFEST_FORMATS") | ||
| public void stripsTrailingSlashFromTableLocation(FileFormat format) throws IOException { |
There was a problem hiding this comment.
It would be good to add a test that shows preserving non-standard paths (e.g. datafile with / or // at the beginning).
There was a problem hiding this comment.
Added preservesNonStandardDataFileLocation() to test this.
| private TrackedFile copyResolved(TrackedFile trackedFile) { | ||
| TrackedFileStruct copy = (TrackedFileStruct) trackedFile.copy(); | ||
| if (copy.location() != null) { | ||
| copy.setLocation(LocationUtil.resolveLocation(tableLocation, copy.location())); |
There was a problem hiding this comment.
Seems like we could make this lazy, but I assume that if we're projecting location we're going to pay the cost at some point. If we want to switch to a reuse based approach, that would be more efficient (e.g. push the table location down and only resolve with location is called on the tracked file.)
There was a problem hiding this comment.
We are already doing a copy, and this PR doesn't make anything worse. Yes, we can always change this in future to make this lazy/zero copy.
|
cc @stevenzwu in case he wanted to take a look. |
2 participants
V4 manifests may store file locations relative to the table location. Resolve relative data file, deletion vector, and leaf manifest locations against the table location when reading, so callers always see absolute paths. The table location is required and absolute paths pass through unchanged.