[adityaksolves]

Closes #18482

RewriteManifests.addManifest did not check the format version of the manifest it was given. On a v2+ table, a v1-format manifest was mishandled on both paths:

• Reused manifest (snapshot ID null): the commit succeeded, but scan planning later threw an NPE in DeleteFileIndex.forEntry because EXISTING entries had null sequence numbers.
• Copied manifest (snapshot ID -1): the commit failed with IllegalStateException: Found unassigned sequence number.

This change rejects v1 manifests up front on v2+ tables with IllegalArgumentException: Cannot add v1 manifest to a v<N> table: <path>. The format version is read from the format-version key in the Avro manifest header (absent means v1). Non-Avro (Parquet) manifests are skipped, since v1 manifests are always Avro.

Tests: added testAddV1ManifestToV2PlusTable and testAddV1CopiedManifestToV2PlusTable in TestRewriteManifests, covering both paths on v2+. Both fail without the fix.

Note: on v2+ tables, each addManifest call now reads the manifest header once.

[adityaksolves]

Hi @amogh-jahagirdar @RussellSpitzer, could one of you take a look when you get a chance?

This fixes #18482, where RewriteManifests.addManifest mishandles v1-format manifests on v2+ tables (NPE during scan planning on the reused path, IllegalStateException on the copied path).

One design question where I'd value your input: I went with rejecting v1 manifests up front on v2+ tables with a clear IllegalArgumentException, rather than copying them and writing the spec default of 0 as explicit sequence numbers (the approach iceberg-cpp takes in apache/iceberg-cpp#774). My reasoning is that it fails fast and avoids silently upgrading the manifest format, but I'm happy to switch to copy-with-0 if that's preferred for Java.

I've rebased onto the latest main and CI is green. Thanks!

[adityaksolves]

@amogh-jahagirdar @RussellSpitzer Could one of you please take a look at this PR when you have time?

It's rebased on latest main with CI green (40/40). The change makes RewriteManifests.addManifest reject v1-format manifests on v2+ tables with a clear IllegalArgumentException, instead of the current NPE during scan planning (reused path) or IllegalStateException (copied path). Both paths have tests that fail without the fix.

I'd especially appreciate your input on the design choice from my earlier comment (fail fast vs. copying with the spec default sequence number 0). Happy to switch if you prefer the other approach.

Thanks!

[RussellSpitzer]

Can you explain what the issue was? I'm a little confused about the NPE since a V2 table should be allowed to have V1 Manifests in it. I think it probably could make sense to not allow adding manifests that aren't from the current table format version, but it probably shouldn't break anything?

[adityaksolves]

Thanks @RussellSpitzer, that’s a fair question. You’re right that a v2+ table can legitimately hold v1 manifests, e.g. ones carried over from an upgrade, and those scan fine.

The failure is specific to adding a v1 manifest through RewriteManifests.addManifest. Repro (formatVersion 2, 3 and 4): build a v1 manifest with EXISTING entries, deleteManifest(original).addManifest(v1Manifest).commit(), then table.newScan().planFiles(). The commit succeeds, but planning throws:

java.lang.NullPointerException: Cannot invoke "java.lang.Long.longValue()" because the return value of "org.apache.iceberg.ManifestEntry.dataSequenceNumber()" is null
    at org.apache.iceberg.DeleteFileIndex.forEntry(DeleteFileIndex.java:151)
    at org.apache.iceberg.ManifestGroup.lambda$createFileScanTasks$14(ManifestGroup.java:400)

Before the commit the manifest is unassigned (sequenceNumber=-1, snapshotId=null); after the commit it gets sequenceNumber=2, snapshotId=2 in the manifest list. A v1 manifest has no sequence number column, so its entries rely on inheritance, and InheritableMetadataFactory only inherits the manifest’s sequence number when it is 0 or the entry is ADDED. Here the entries are EXISTING and the manifest sequence number is 2, so neither holds and dataSequenceNumber stays null. A v1 manifest carried over from an upgrade keeps sequence number 0, which is why that case works.

So I agree v1 manifests on v2+ tables are fine in general; the problem is only this add path. I went with rejecting because it fails fast, but I’m happy to switch to rewriting the manifest with sequence number 0 for its entries, as iceberg-cpp does (apache/iceberg-cpp#774), if you’d prefer that. Which would you prefer?