Core: Read delete manifests correctly during snapshot expiration

[dramaticlly]

2 bugs make it right:

1. FileCleanupStrategy#readManifests projects the manifest list without the content field (517) and GenericManifestFile defaults content to DATA and only assigns it when the field is projected, so every manifest handled by ReachableFileCleanup and IncrementalFileCleanup reports ManifestContent.DATA, including delete manifests.

2. ManifestFiles.readPath open reads data manifest and ManifestReader does not derive its schema from FileType.

Clean up of delete manifest as part of snapshot expiration is already covered today in

iceberg/core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java

Line 1040 in 7f879b1

public void testExpireWithDeleteFiles() {

[dramaticlly]

read() only handles data manifests while open() handle both data and delete manifests

[dramaticlly]

@amogh-jahagirdar @szehon-ho can you help take a look?

[szehon-ho]

Approving, the fix looks correct to me.

One question: does the existing coverage you linked (TestRemoveSnapshots#testExpireWithDeleteFiles) actually reproduce either bug? From reading it I think it passes with and without this change, since the two bugs cancel each other out for path collection. Left a test suggestion inline that would catch a regression in the projection.

[szehon-ho]

Can we add a test that fails without this line? Reverting just this field while keeping the readPaths change leaves every test green: open() sees DATA, dispatches to read(), and reading file_path still works. Both cleanup paths read under suppressFailureWhenFinished, so a regression here shows up as a warning and leaked delete files rather than a failure.

Something like this, which also covers DVs in v3 and Parquet manifests in v4:

@TestTemplate
void readManifestsProjectsManifestContent() throws IOException {
  assumeThat(formatVersion)
      .as("Delete files are only supported in V2 and later")
      .isGreaterThanOrEqualTo(2);

  table.newAppend().appendFile(FILE_A).commit();
  table.newRowDelta().addDeletes(fileADeletes()).commit();

  Snapshot snapshot = table.currentSnapshot();
  Map<String, ManifestContent> expected =
      snapshot.allManifests(table.io()).stream()
          .collect(Collectors.toMap(ManifestFile::path, ManifestFile::content));
  assertThat(expected).containsValue(ManifestContent.DELETES);

  FileCleanupStrategy cleanup = new ReachableFileCleanup(table.io(), null, null, null);

  Map<String, ManifestContent> actual = Maps.newHashMap();
  try (CloseableIterable<ManifestFile> manifests = cleanup.readManifests(snapshot)) {
    for (ManifestFile manifest : manifests) {
      actual.put(manifest.path(), manifest.content());
    }
  }

  assertThat(actual).isEqualTo(expected);
}

Note the containers are reused, so path() and content() have to be read inside the loop rather than collecting the ManifestFile references.

Or if the test catches the bug , feel free to ignore.

[dramaticlly]

Thanks @szehon-ho , added UT as suggested for coverage on MANIFEST_PROJECTION.

Unlike existing UT, we do need to create a package private ReachableFileCleanup and calling protected method to exercise the projection logic. Also improve the coverage on v3 and v4 format in TestRemoveSnapshots.

Credit to @dkranchii for identifying this problem first in #17745 and #17772 adds the V3 parameterization in TestRemoveSnapshots, I added him as coauthor in the rebased commit