StructLike classes in Iceberg often don't implement the equals method. but in this case, I am ok with it for usage in the TrackedFileBuilder

I think that we should remove this. There are multiple ways of considering something "equal" and we try to avoid equals methods when it is unclear which one should be used. That's the case here, where it would be reasonable to assume that two DeletionVector instances are the same if they represent the same DV (location and offset are equal) but also reasonable to assume two instances are equal if they have the same values (mostly implemented here).

In addition to the problem of what this means, there's another problem that this definition only works for other DeletionVectorStruct instances. That means the definition of equality is more murky: two DeletionVector instances can be equal by either definition above but still may not be equal here. The purpose of using interfaces for the public API is to make it so that implementations are interchangeable and this breaks that principle.

I think this was introduced to be able to detect changes applied to the builder, but we should find a better way to solve that problem rather than introducing this.

@rdblue the comparison was introduced due to my comment here. Do you think it is sth we should guard against? if not, we can maybe just add Javadoc to explain the usage. if yes, should we just do a reference comparison like this.deletionVector != newDeletionVector or we want a util method like `DelectionVectorUtil.sameContents(DeletionVector, DeletionVector)?

I dropped these overrides here, and went for the verification suggested by Ryan within the builder class.

Would this ever be null? I'm starting from the assumption that in v4 we will only write v4 metadata files. If that's true, then I think there are two cases when building a TrackedFile instance: either we are writing a v4 file or we are adapting an existing v1-3 file to be tracked in v4. The first case is easy: always set this to 4.

Does the second case go through this builder? I would expect to use a ManifestFile to TrackedFile adapter, like the adapters @anoopj introduced for TrackedFile to DataFile, DeleteFile, etc. That is a good pattern to follow because we never want a caller that is moving tracked files around to have the logic for migrating from v3 to v4.

yes, I have a follow-up PR (draft status now) to adapt v3 shapes (like ManifestFile and DataFile) to v4 TrackedFile. It would be good to use builder for those adaptions too, unless you are thinking about using the constructor directly.

Regarding the null or not, current proposal/spec PR said it is not null (0 for pre-v4 and format versions for v4+)

Thinking about this more, would we use adapters to wrap DataFile and DeleteFile anyway? Those are what our writers produce already and I doubt that we will want to go through and update all the plumbing to produce TrackedFile directly. The utility of this class may be very limited.

I could see us producing instances of TrackedFile by reading and wrapping v3 DataFile instances, or wrapping a new DataFile instance for v4. We should verify there are cases where we would use this.

Apparently, as Steven noted, we could use the TrackedFileBuilder for the wrappers around V3 metadata structs, and there we could use this writer_format_version field through the builder. See the draft PR linked by Steven and let me know if it makes sense.

I believe we concluded on (writer_)format_version related question on the other dedicated PR. Let me know if there is still something outstanding here!
Checking again Steven's PR on the wrappers, I think it makes sense to set this through the builder.

PartitionData should be StructLike. We don't want to rely on concrete classes here.

Also, this is in the "required fields" section, but I think that we want to allow this to be null for unpartitioned cases. The partition tuple will be null when specId is null or an unpartitioned spec, because the actual partition type that we store is the union schema of all valid partition specs in the table. And support for empty struct types and instances is unreliable.

This is less direct than we typically prefer, but is not bad. This states a general rule and then says that it applies. We would normally say that at once: "Cannot transition %s to REPLACED".

Thx! I think we can even remove the content type parameter of the message:
Cannot transition manifest files to REPLACED

We should not need unchecked casts like this.

The difficulty here is the TrackedFile.partition() return StructLike while the TrackedFileStruct constructor accepts PartitionData. Since internally in TrackedFile we also have PartitionData this seemed safe

I think that this should be combined with specId: partition(int newSpecId, StructLike partitionTuple). If the tuple is set, then spec ID is required so that we know how to interpret it. I suspect that this should also be responsible for wrapping the partition tuple with a projection to the union type.

I probably miss something here, but I see that TrackedFile has a PartitionData member. If we want to produce that, is specId and partitionTuple enough? Don't we need a PartitionSpec to produce PartitionData?

"Cannot set sort order for manifest files"?

Fixed

"Cannot add deletion vector for file with content: %s"

Fixed, and found couple other similar messages, fixed them too.

Style: this. is used when assigning to a field, but not when accessing a field.

Fixed

The check that I would expect is deletionVector == null || newDeletionVector.cardinality() > deletionVector.cardinality(). That ensures that the deletion vector was updated, without relying on an equality check. Otherwise, bad data could still make it through just by using a different concrete class (which is not equal).

The cardinality check is closer to what we want although not foolproof. The underlying location and offset could be the same, although it is unlikely that would be the case. If you want to make sure that this is actually a different deletion vector, then I think the right thing to do is to do that check here:

Preconditions.checkArgument(
    deletionVector == null ||
        deletionVector.cardinality() < newDeletionVector.cardinality(),
    "Invalid DV update, cardinality must increase: existing=%s, new=%s", ...);
Preconditions.checkArgument(
    deletionVector == null ||
        !deletionVector.location().equals(newDeletionVector.location()) ||
        !deletionVector.offset().equals(newDeletionVector.offset()),
    "Invalid DV update: same offset and location has changed cardinality");

Thanks, borrowed these checks!

I'm not against this since I haven't seen how it is used yet. But I find these methods that capture data that will be passed through to Tracking suspicious. Why not pass in a new Tracking and not mirror its builder's API here? Also, if this is the pattern we want, then why not wrap a TrackingBuilder and pass this directly?

Felt a bit odd for me when I implemented this but went this way for two reasons:

1. I tried to avoid allow passing Tracking in the TrackedFileBuilder for the users because similar reasons we don't allow passing status for TrackingBuilder: seems to represent an internal state and users might get it wrong to construct. We could guard against malformed Tracking objects with checks, but seemed that we already have everything to construct it internally and not push one more responsibility to the users of the builder.
2. TrackingBuilder itself can't check for all of the constraints. For instance it doesn't know if it belongs to a data file or a delete manifest, hence can't verify that deleted/replaced positions are only allowed for manifests. We can perform such a check here, in the TrackedFileBuilder.

Introduced a TrackingBuilder member to the TrackedFileBuilder and we can delegate all these calls right away without keeping track the required information.

I don't think this is correct.

Thx!
I change partition not to be required through the builder