Core: RewriteManifests.addManifest mishandles v1-format manifests on v2+ tables #18482

Apache Iceberg version

main (development). Reproduced at db43b88; the same code paths are in 1.12.0 (latest release).

Query engine

None (core API).

Please describe the bug 🐞

RewriteManifests.addManifest does not check the format version of the manifest it is given. On a v2+ table, a manifest written in v1 format (no sequence_number / file_sequence_number columns) is mishandled on both paths.

Reused manifest (snapshotId() is null, snapshot ID inheritance enabled): the commit succeeds and the v1 manifest is added to the manifest list as-is, with the new commit's sequence number. On read, BaseInheritableMetadata only fills in sequence numbers for ADDED entries or when the manifest's sequence number is 0, so the EXISTING entries come back with null data and file sequence numbers. Scan planning then throws a NullPointerException, because DeleteFileIndex.forEntry passes entry.dataSequenceNumber() to forDataFile(long, DataFile).

Copied manifest (snapshotId() is -1): ManifestFiles.copyRewriteManifest reads the entries with empty inheritable metadata, so the sequence numbers stay null and the v2+ writer fails inside addManifest with:

IllegalStateException: Found unassigned sequence number for an entry from snapshot: <id>

The spec says that "when reading v1 manifests with no sequence number column, sequence numbers for all files must default to 0", and that existing entries written to a new manifest must carry non-null sequence numbers.

Reproduction

In TestRewriteManifests with format version 2 or later:

table.newFastAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
Snapshot first = table.currentSnapshot();
ManifestFile original = Iterables.getOnlyElement(first.allManifests(table.io()));

OutputFile out = table.ops().io().newOutputFile(temp.resolve("v1-manifest.avro").toString());
ManifestWriter<DataFile> writer = ManifestFiles.write(1, table.spec(), out, null);
try {
  writer.existing(FILE_A, first.snapshotId(), first.sequenceNumber(), first.sequenceNumber());
  writer.existing(FILE_B, first.snapshotId(), first.sequenceNumber(), first.sequenceNumber());
} finally {
  writer.close();
}

table.rewriteManifests().deleteManifest(original).addManifest(writer.toManifestFile()).commit();

// the committed entries read back with null data and file sequence numbers
Lists.newArrayList(table.newScan().planFiles()); // NullPointerException

Writing the manifest with snapshot ID -1L instead of null takes the copy path, and addManifest throws the IllegalStateException above. Both behaviors reproduce on format versions 2, 3 and 4.

Expected behavior

addManifest should handle v1-format manifests on v2+ tables consistently: either reject them with a clear error, or copy them and write the spec default of 0 as explicit sequence numbers. It should not commit a manifest that breaks scan planning.

This came up while reviewing the same operation in iceberg-cpp (apache/iceberg-cpp#774), which copies v1-format manifests and writes sequence number 0.