stevenzwu commented on code in PR #17434:
URL: https://github.com/apache/iceberg/pull/17434#discussion_r3708146056
##########
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:
> Requiring immutability results in a lot of object thrashing.
@danielcweeks this doesn't require additional object copy/thrashing.
Currently, we do `trackedFile.copy()` first, then adjust the location
fields. What I suggested is also one copy
`copyWithResolvedLocation(tableLocation)`. There is no difference in terms of
memory allocation.
> I'm a bit wary of changing the semantics of the copy constructors to now
do location resolution.
To clarify: the copy constructor stays as a mechanical field installer — it
takes the already-resolved values as parameters, not `tableLocation` itself.
The resolution lives one layer up in a new `copyWithResolvedLocation(String
tableLocation)` method that computes the resolved values and invokes the
constructor:
```java
// copy constructor: pure field installer, does not know about LocationUtil
private TrackedFileStruct(
TrackedFileStruct toCopy,
Set<Integer> statsIds,
String resolvedLocation,
DeletionVector dvWithResolvedLocation) {
super(toCopy);
...
this.location = resolvedLocation;
...
this.deletionVector = dvWithResolvedLocation;
...
}
// new caller-facing entry point that resolves before installing
TrackedFile copyWithResolvedLocation(String tableLocation) {
String resolvedLocation =
location != null ? LocationUtil.resolveLocation(tableLocation,
location) : null;
return new TrackedFileStruct(
this, null, resolvedLocation, resolveDvLocation(deletionVector,
tableLocation));
}
private static DeletionVector resolveDvLocation(DeletionVector dv, String
tableLocation) {
return DeletionVectorStruct.builder()
.location(LocationUtil.resolveLocation(tableLocation, dv.location()))
.offset(dv.offset())
.sizeInBytes(dv.sizeInBytes())
.cardinality(dv.cardinality())
.build();
}
```
`V4ManifestReader.copyResolved` then collapses to
`trackedFile.copyWithResolvedLocation(tableLocation)`.
> a third option: NOT do location resolution in this layer, and instead do
it lazily in scan planning time, in the adapter layer in the respective
`path()` methods
I like the location resolution in this layer. A single place. We can do the
same thing on the write direction like `V4ManifestWriter`.
> the objects are not immutable btw since they expose positional setters
anyway
Agree they are not immutable. But the positional setter is a framework hook
for the Avro/Parquet reader to populate fields by ordinal during
deserialization — a construction-time protocol, not a post-construction
mutation API. A named `void setLocation(String)` looks like a domain-level
"mutate this specific field" method that any package member could call. The
concern is probably more on the API shape.
But since these are non-public setters, I am ok with it.
`BaseFile.setManifestLocation` is a similar pattern.
--
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]
