[anoopj]

Add TrackedFileAdapters.asManifestFile to wrap a DATA_MANIFEST or DELETE_MANIFEST TrackedFile as a ManifestFile.

The ManifestFile adapter will only be used by tests. Note that ManifestFile doesn't support manifest DVs. So in the actual manifest read path, the reader resolves the MDV first and produces the resulting TrackedFiles, which are then adapted to data/delete files.

Closes #16227

[wombatu-kun]

Code looks correct: field delegation, equals/hashCode (consistent with GenericManifestFile), constructor preconditions, and the test coverage are all solid.

Non-blocking discussion point: partitionSpecId() throws UnsupportedOperationException. That is defensible since a v4 manifest is not bound to a single spec. But @rdblue pushed back on #16100 against these adapters exposing an invalid or arbitrary spec ID, so it may be worth confirming with reviewers that a throw is the intended contract for any future caller that reads partitionSpecId().

[stevenzwu]

we can use the builder from @gaborkaszab 's PR: #16769

[anoopj]

Sounds good. I can make the change if #16769 gets merged first or do this as a followup PR if this gets merged first.

[gaborkaszab]

FYI, in the meantime the builder got merged

[gaborkaszab]

Thank you for the PR, @anoopj !
I took a Quick Look and left some minor comment.

[gaborkaszab]

nit: other adapters don't check if tracking is null. Not sure we have to here. It's a required field anyway

[anoopj]

Removed.

[gaborkaszab]

As per the spec proposal data seq num is 'Optional for leaf manifests, required for root.'. I think it's valid to have it as null here. Seems also optional in ManifestFile

[anoopj]

This PR is for root-level manifest tracking where null is truly an invalid state. We can either validate or just have it fail with an NPE.

Seems also optional in ManifestFile

It is required for a manifest file: ManifestFile.sequenceNumber() returns a primitive

[stevenzwu]

just to add on top of what Anoop said. this is for leaf manifest file entry (TrackedFile) in the root manifest file. Hence dataSequenceNumber should be required

[gaborkaszab]

Thanks for the explanation @anoopj and @stevenzwu !

[gaborkaszab]

The other adapters don't override equals and hashCode. Is there a particular reason it's needed here?

[anoopj]

I'm trying to be consistent with the semantics of GenericManifestFile.

[rdblue]

Do we actually use equals and hashCode? We try to avoid implementing those methods where there are different interpretations of what "equality" might mean.

[anoopj]

I didn't add any code that uses it. I was trying to be consistent with the behavior of the GenericManifestFile which defines equality with the location. I have removed it.

[stevenzwu]

LGTM.

I will merge Gabor's TrackedFileBuilder once CI passed. then let's rebase this one and use the builder as applicable.

[stevenzwu]

just to add on top of what Anoop said. this is for leaf manifest file entry (TrackedFile) in the root manifest file. Hence dataSequenceNumber should be required

[rdblue]

Nit: I think we prefer the slightly more specific "Invalid data sequence number: null" because "no X" doesn't necessarily mean "x is null".

Also, even though the attempt is to create a manifest file, that's not the actual problem. It's debatable whether to include what is being attempted, "Cannot create manifest file", but I would opt to leave it out because that's clear from other context (that the check is in asManifestFile and the TrackedManifestFile constructor).

[anoopj]

Done

[rdblue]

I don't think that it is correct to say "Tracked manifests": what is a "tracked" manifest and why is one from a manifest list not "tracked"?

I think it is better to say "v4 manifests are not bound to a single partition spec".

[anoopj]

Fixed. I was trying to avoid having too many v4 references.

[rdblue]

Minor: I think it is safer to avoid a dependency between this and the constructor's verification. I would recommend using a switch statement:

switch (file.contentType()) {
  case DATA_MANIFEST:
    return ManifestContent.DATA;
  case DELETE_MANIFEST:
    return ManifestContent.DELETES;
  default:
    throw new UnsupportedOperationException("Unsupported content type for manifests: " + file.contentType());
}

[anoopj]

Done

[rdblue]

I don't think that the implementation of GenericManifestFile is very helpful. It considers two files equal if they have the same path (which I think is reasonable) but it also excludes any implementation other than GenericManifestFile, which means that they aren't actually interchangeable as the interface suggests.

In addition, this creates a situation where a.equals(b) may be true, but b.equals(a) can be false for reasonable implementations.

I'd prefer to remove this and fix places where it is assumed or used. Good catch, @gaborkaszab!

[anoopj]

Removed.

[gaborkaszab]

This also seems an internal invariant for TrackedFile. The new TrackedFileBuilder guarantees that manifestInfo is set for manifest entries.

[anoopj]

That makes sense. Removed.

[gaborkaszab]

FYI, in the meantime the builder got merged

[gaborkaszab]

Thanks for the explanation @anoopj and @stevenzwu !

[gaborkaszab]

nit: Can we hide the construction of the input TrackedFile into a separate function? Like dataManifestFile() it could get ManifestInfo, FileContent, and/or Tracking parameters if that matters for the test.

[anoopj]

Done. extracted manifestBuilder(FileContent) / manifestFile(FileContent, ManifestInfo) and now tests build through them.

[gaborkaszab]

nit: maybe parameterize the test for data + delete manifest. The testDeleteManifestContentMapsToDeletes test could be dropped then

[anoopj]

good suggesiton. Done.

[gaborkaszab]

just some minor comments. Other than them, LGTM!

[gaborkaszab]

Just FYI: in the follow-up for the TrackedFileBuilder PR, we decided to drop populateTrackingFields and went to call the constructor of TrackedFile instead using the createTracking (revived in the follow-up PR) method.

[anoopj]

Ack. I can rebase when the followup PR is merged.

[gaborkaszab]

instead of using the ordinal, we can use the inheritFrom(some_manifest) method to do that.

[anoopj]

Routing through inheritFrom() will require building a parent Tzracking with several nulls. I prefer to keep it since this is simpler.

[rdblue]

If comments require regular maintenance when touching the file, I prefer to rewrite them to minimize churn. This doesn't need to list everything that can be produced. Maybe edit this so that it say for example, {@link DataFile}?

[rdblue]

This is no longer true. How about "Shared base for data and delete file adapters"?

[rdblue]

I think this should be a constant, not a method. We don't need to create a manifest info struct every time, and a method gives no guarantees about the values produced. It could create random values every time.

If we use a constant, then you can write assertions like assertThat(manifest.existingFilesCount()).isEqualTo(manifestInfo.existingFilesCount()).

[rdblue]

I don't think that the complexity here to use the builder is a good thing.

This creates indirection and makes tests harder to read, while gaining almost nothing. There is hardly any readability benefit to using the builder when you're already setting constants like FORMAT_VERSION_V4. All this does is move the initialization here, so people have to find this method to validate that the assertions above are correct, like assertThat(manifest.path()).isEqualTo(MANIFEST_FILE_LOCATION). I would rather see MANIFEST_FILE_LOCATION passed in and then validated.

I think this should be removed and the constructor should be used instead. All these tests need to verify is that these values are passed through correctly.

[rdblue]

Please remove this and either use a Tracking constant or create one inline. This indirection makes the tests unreadable.

[rdblue]

This is correct and calls into question whether this adapter is going to be useful. We expect manifest DVs to be common and if we can't adapt a manifest with a DV back to ManifestFile, then what is the purpose of this adapter?

ManifestFile is mostly below the API level. It is exposed in some places (accessed primarily from Snapshot), but reading and writing for correctness (scan planning and commit operations) are all done within Table APIs and even snapshot change access is moving to SnapshotChanges. You can read manifests directly using utils in core, but I don't think that is very common.

I think we may want to avoid adding this for now, even though it seems like a reasonable addition. In order for this to be valuable, we would need to continue using ManifestFile and expose a DV through that interface. Since we don't know if that's going to be helpful, I think we should keep this as a draft for now.

[anoopj]

OK. I will convert this into a draft and we can revisit this if it's needed as we build the reader and writer. cc @stevenzwu

[stevenzwu]

In the write path, I also proposed new additions to the ManifestFile interface. I added 3 new methods in my next PR #16936 . We can add more (including manifest DV) as we go.

[anoopj]

If we add the DV, let me know and I can revive this PR.

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

[github-actions]

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.