stevenzwu commented on code in PR #17434:
URL: https://github.com/apache/iceberg/pull/17434#discussion_r3707149036
##########
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.
Although we may not have the thread safety issue with the setter approach
here, I would still love to maintain the immutability of `TrackedFileStruct`.
Maybe we can extend the copy constructor to include the two new resolved
objects: location and dv. In the future, we can further extend the copy
constructor to handle column files too.
```
/** Copy constructor. */
private TrackedFileStruct(TrackedFileStruct toCopy, Set<Integer> statsIds,
String resolvedLocation, DeletionVector dvWithResolvedLocation)
```
We can also extend/rename the `TrackedFile.copyWithStats`
```
TrackedFile copyWithStatsAndResolvedLocation(Set<Integer>
requestedColumnIds, String tableLocation)
```
I am wondering if there is any actual need of `TrackedFile.copyWithStats`.
The stats projection is achieved as part of the `InternalReader` projection. If
we don't need it, we can simplify the above API as.
```
TrackedFile copyWithResolvedLocation(String tableLocation)
```
--
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]
