manuzhang opened a new issue, #18482:
URL: https://github.com/apache/iceberg/issues/18482
### Apache Iceberg version
main (development). Reproduced at db43b88a6c; the same code paths are in
1.11.0.
### 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:
```java
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
