anoopj commented on code in PR #17434:
URL: https://github.com/apache/iceberg/pull/17434#discussion_r3708016434
##########
core/src/main/java/org/apache/iceberg/V4ManifestReader.java:
##########
@@ -146,6 +151,22 @@ private TrackedFile prepare(TrackedFile trackedFile) {
return trackedFile;
}
+ // resolves stored locations against the table location
+ private TrackedFile copyResolved(TrackedFile trackedFile) {
+ TrackedFileStruct copy = (TrackedFileStruct) trackedFile.copy();
Review Comment:
> should we have the open method just return
CloseableIterable<TrackedFileStruct>? that might save one type cast here.
That sounds good.
> Maybe we can extend the copy constructor to include the two new resolved
objects: location and dv.
I'm a bit wary of changing the semantics of the copy constructors to now do
location resolution. Also, the objects are not immutable btw since they expose
positional setters anyway.
I can see a third option: which is to *NOT* do location resolution in this
layer, and instead do it in scan planning time, in the adapter layer in the
respective `path()` methods. e.g.
[here](https://github.com/apache/iceberg/blob/58d5c3704f1ea55a38383d10e2bfbb947d968b3f/core/src/main/java/org/apache/iceberg/TrackedFileAdapters.java#L131-L132).
cc @danielcweeks in case he has a suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
