[PR] Core: Reject v1 manifests in RewriteManifests.addManifest for v2+ tables [iceberg]

["adityaksolves (via GitHub)" <gi...@apache.org>]

adityaksolves opened a new pull request, #18502:
URL: https://github.com/apache/iceberg/pull/18502

   Fixes #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.


-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] Core: Reject v1 manifests in RewriteManifests.addManifest for v2+ tables [iceberg]

["adityaksolves (via GitHub)" <gi...@apache.org>]

adityaksolves commented on PR #18502:
URL: https://github.com/apache/iceberg/pull/18502#issuecomment-6362942710

   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!


-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org