Core: Resolve relative paths in V4 manifest reader

[anoopj]

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.

[anoopj]

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.

[danielcweeks]

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.

[anoopj]

Thanks for confirming.

[anoopj]

cc @stevenzwu in case he wanted to take a look.

[stevenzwu]

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)

[anoopj]

should we have the open method just return CloseableIterable? 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 lazily in scan planning time, in the adapter layer in the respective path() methods. e.g. here.

cc @danielcweeks in case he has a suggestion.

[danielcweeks]

Although we may not have the thread safety issue with the setter approach here, I would still love to maintain the immutability of TrackedFileStruct.

@stevenzwu, immutability is not really a expected characteristic here. In fact, we'll likely move to a model where we reuse the objects (prior readers heavily leverage reuse to improve performance). Requiring immutability results in a lot of object thrashing.

I think there are other ways to optimize but the cases where it adds value are really narrow and we should focus on those later if they actually become a problem.

[stevenzwu]

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:

// 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 (for contextual info - not an actual column though).