Core: Add v4 TrackedFileAdapters to bridge Data/Delete Files

[anoopj]

The adapter bridges TrackedFile to existing DataFile/DeleteFile APIs and would allow to minimize the v4 related code changes during scan planning and commits.

Fixes #16221

[anoopj]

This will change after the approach to store partition tuple is settled.

[rdblue]

Steven and Russell replied on the dev list that they're good with a tracked partition tuple. I also confirmed with Dan and Amogh, so I think we are probably good to go. Maybe we should start getting the changes to the interfaces and implementations done? This would then only need to run the projection.

[anoopj]

An open question is whether it's worth caching the stats (lazy/eager). I don't see a lot of repeated reads, so may not be worth it.

[rdblue]

It's probably fine. I have a comment about this below.

[rdblue]

What was your reason not to make this a method of TrackedFile?

Also, PartitionSpec is tracked by the file itself, so is very strange to pass it in here. At a minimum, I would expect this to have a validation that the spec's ID matches the file's spec ID. But it would also be better to have some way to look up spec by ID instead of forcing the caller to do it and then validate that the caller did it correctly.

[anoopj]

I intentionally kept the adapters outside of TrackedFile to avoid coupling it with Data/DeleteFile. It seemed like an adapter concern.

The tracked file keeps track of the spec ID, but not the spec itself. I saw some v3 code paths that followed a similar pattern of passing around specsById. Pretty much open to changing it if you have suggestions.

[anoopj]

I changed the methods to take in specsById map instead.

[rdblue]

I appreciate the comment. The decision here seems reasonable to me since we don't know the total Puffin file size.

We should also consider whether we want to have a field in the tracked_file struct for this. We originally wanted to use file_size_in_bytes for the Puffin file size so that we could determine when DVs should be compacted. But variance in the footer was a problem that prevented it from being used as intended.

@aokolnychyi should we revisit this?

[stevenzwu]

even if we know the total Puffin file size, dv.sizeInBytes still seems the correct value here. A puffin file may contain thousands of DVs. A logical DeleteFile should only contain one DV. The DeleteFile size should be the DV size.

[rdblue]

This is okay, but a little concerning because the helper method here creates a new map every time it is called. Creating the map is an expensive operation because it allocates buffers to hold each column bound and serializes into that buffer. Then the map is thrown away rather than reused.

The evaluators themselves (for example, InclusiveMetricsEvaluator) only call these methods once to evaluate for the data file, but if multiple evaluators are used then we should expect a performance degredation.

On the other hand, we want to have evaluators that work directly with ContentStats instead of going through these methods. I think I'm fine with leaving this as-is for now, but we need to make sure that we use the right evaluators everywhere.

[anoopj]

Agree. Will build a new evaluator that operates on content stats, as a followup. cc @nastra

[anoopj]

Filed #16218 to track this.

[rdblue]

Let's hold off on doing it right now. I think the ContentStats API is going to change.

[rdblue]

We will also need a way to wrap TrackedFile as a v2 DeleteFile for position deletes.

[anoopj]

Ack. I will create a followup PR for this so that the size is manageable.

[anoopj]

Added TrackedPositionDeleteFile

[rdblue]

Rather than Abstract, we typically use Base for the prefix. For example, BaseAction or BaseContentStats.

[stevenzwu]

I think we can even drop the Base here with just TrackedContentFile, because ContentFile is a base class. This is also consistent with other classes like TrackedDataFile, TrackedDVDeleteFile.

[anoopj]

I dropped the Base. @rdblue let me know if thats OK.

[rdblue]

It would be helpful to implement the version for v2 position deletes here as well. That would make it easier to evaluate the implementations here, although I suspect it will be fine.

[anoopj]

Added it. It's actually fairly light.

[stevenzwu]

switch to the builder added recently?

[nastra]

+1 as I feel setting all stuff by position is quite brittle

[anoopj]

Done.

[stevenzwu]

I think we can even drop the Base here with just TrackedContentFile, because ContentFile is a base class. This is also consistent with other classes like TrackedDataFile, TrackedDVDeleteFile.

[stevenzwu]

Is this class only for EQUALITY_DELETES, as the content() method override indicates? if yes, it can probably names as TrackedEqualityDeleteFile

Just to confirm my understanding. This is for new V4 manifest files, where v2 position delete file entries won't exist.

[anoopj]

Is this class only for EQUALITY_DELETES, as the content() method override indicates? if yes, it can probably names as TrackedEqualityDeleteFile

Correct. Renamed.

Just to confirm my understanding. This is for new V4 manifest files, where v2 position delete file entries won't exist.

v2 position delete file entries can also exist, if a table is upgraded from v2. I just added TrackedPositionDeleteFile to handle it. PTAL

[stevenzwu]

even if we know the total Puffin file size, dv.sizeInBytes still seems the correct value here. A puffin file may contain thousands of DVs. A logical DeleteFile should only contain one DV. The DeleteFile size should be the DV size.

[gaborkaszab]

I went through this for my own education. Left some comments, let me know if they make sense.

[gaborkaszab]

nit: The only thing needed from TrackedFile here is the specId. Can't we pass that instead, and then there is nothing V4 specific in this function (like TrackedFile, etc.) so it might go into some PartitionSpec related util class.

[anoopj]

That is true, but I think this signature makes the call sites a bit more readable.

[gaborkaszab]

nit: Isn't this redundant? Tracking is part of TrackedFile

[anoopj]

Changed to just delegate to file.tracking()

[gaborkaszab]

Doesn't this comment mean that practically we shouldn't have a TrackedFile member on this level of abstraction?

[anoopj]

The comment was a bit stale. Fixed. Thanks for the callout!

[gaborkaszab]

nit: the default implementation already returns null. Isn't this redundant here? Same for content() and some other functions elsewhere like firstRowId()

[anoopj]

Removed the ones that were returning null. Kept content() override to be explicit.

[stevenzwu]

This justification doesn't hold. In TrackingStruct.java, BASE_TYPE (L33-43) already includes MetadataColumns.ROW_POSITION as its 9th field, and the builder's build() calls a constructor that does super(BASE_TYPE, BASE_TYPE) (L91), so builder-created structs do support row position projection.

manifestLocation and manifestPos aren't part of the struct projection at all - they're plain instance fields (L55-56), "set by manifest readers, not written to manifests". They're assigned via setManifestLocation(...) and set(8, ...) regardless of which constructor was used.

So the helper can just be:

TrackingStruct tracking = TrackingStruct.builder()
    .status(EntryStatus.ADDED)
    .snapshotId(42L)
    .dataSequenceNumber(10L)
    .fileSequenceNumber(11L)
    .firstRowId(1000L)
    .build();
tracking.setManifestLocation("s3://bucket/manifest.avro");
tracking.set(8, manifestPos);
return tracking;

[anoopj]

I had fixed exactly this last week. Sounds like this was a stale comment?

[stevenzwu]

TrackedPositionDeleteFile doesn't override referencedDataFile(), contentOffset(), or contentSizeInBytes(), so all three fall through to the DeleteFile interface defaults of null. But V4Metadata.java (cases 17/18/19, L495-509) writes those slots out for POSITION_DELETES entries, so V4 manifests can carry V2-style position delete metadata — a V2 delete file with referenced_data_file and content offset/size.

The read-side gap: TrackedFile (interface) doesn't expose referencedDataFile/contentOffset/contentSizeInBytes, so this adapter has no way to surface them even if they were in the manifest. So either:

1. V4 manifests aren't expected to round-trip V2 position delete metadata (worth a code comment here saying these are intentionally always null), or
2. TrackedFile is missing fields for position-delete-specific metadata, and this adapter silently drops them when reading V2 position deletes from a V4 manifest.

The test testPositionDeleteFileAdapterDelegatesAllFields doesn't exercise these three fields, so the gap isn't visible there.

Side note while we're here: TrackedFile.java:37 documents content_type as "0=DATA, 2=EQUALITY_DELETES, 3=DATA_MANIFEST, 4=DELETE_MANIFEST" — 1=POSITION_DELETES is omitted, but this PR's asPositionDeleteFile implies it's valid. Worth updating that doc to match (separate file, but the inconsistency is exposed by this PR).

[anoopj]

I don't believe there is a read-side gap here for v2 positional deletes. These three fields (referencedDataFile, contentOffset, contentSizeInBytes) are v3 DV fields right? TrackedPositionDeleteFile doesn't need to surface them because it is for an actual v2-style positional delete. (which can come from a v2 table upgrade).

V3 DVs can't come through this adapter. V3 DVs live in v3 delete manifests and come out as DeleteFile objects directly from the v3 manifest reader, without any adapter. I was just having an offline conversation with @rdblue about this.

In v4, DVs are always colocated with their data file entries as DeletionVector structs, surfaced through TrackedDVDeleteFile.

Did that make sense?

Good catch on TrackedFile. Fixed.