Spark 4.0: Pass `format-version` when creating a snapshot in table migration actions by Fokko · Pull Request #14163 · apache/iceberg

Fokko · 2025-09-22T21:59:57Z

kevinjqliu · 2025-09-23T03:14:42Z

TestAddFilesProcedure > addFilteredPartitionsToPartitioned() > catalogName = testhive, implementation = org.apache.iceberg.spark.SparkCatalog, config = {type=hive, default-namespace=default}, formatVersion = 1 FAILED
java.lang.IllegalArgumentException: Snapshot id must be assigned during commit
at org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkArgument(Preconditions.java:141)

from

iceberg/spark/v4.0/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java

Lines 551 to 569 in 16fa673

    
           @TestTemplate 
        
           public void addFilteredPartitionsToPartitioned() { 
        
             createCompositePartitionedTable("parquet"); 
        
             createIcebergTable( 
        
                 "id Integer, name String, dept String, subdept String", "PARTITIONED BY (id, dept)"); 
        
             List<Object[]> result = 
        
                 sql( 
        
                     "CALL %s.system.add_files('%s', '`parquet`.`%s`', map('id', 1))", 
        
                     catalogName, tableName, fileTableDir.getAbsolutePath()); 
        
             assertOutput(result, 2L, 1L); 
        
             assertEquals( 
        
                 "Iceberg table contains correct data", 
        
                 sql("SELECT id, name, dept, subdept FROM %s WHERE id = 1 ORDER BY id", sourceTableName), 
        
                 sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName)); 
        
           }

,

iceberg/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/SparkTableUtil.java

Line 912 in 651accb

manifests.forEach(append::appendManifest);

and

iceberg/core/src/main/java/org/apache/iceberg/MergeAppend.java

Lines 57 to 59 in 16fa673

    
           Preconditions.checkArgument( 
        
               manifest.snapshotId() == null || manifest.snapshotId() == -1, 
        
               "Snapshot id must be assigned during commit");

looks like we can relax that constraint; snapshot_id could be null :)

kevinjqliu

LGTM!

kevinjqliu · 2025-09-23T03:19:47Z

+    final Long snapshotId;
+    if (formatVersion == 1 && !snapshotIdInheritanceEnabled) {
+      snapshotId = ops.newSnapshotId();
+    } else {
+      snapshotId = null;
+    }


👍
from the docs, https://iceberg.apache.org/spec/#manifest-entry-fields

Snapshot id where the file was added, or deleted if status is 2. Inherited when null.

amogh-jahagirdar

Thanks @Fokko for the fix, looks right to me! Is it possible to add a test to some either the snapshot action test or table migration test which asserts more state about the manifests that are produced via the procedure? I'd imagine in those tests we should be able to query the metadata tables and assert that some of these fields on the manfiest are what we expect?

Fokko · 2025-09-23T08:01:01Z

@kevinjqliu It was late yesterday, but I wanted to get something out. It looks like the last snipper you shared shows the right approach rather than relaxing constraints:

iceberg/core/src/main/java/org/apache/iceberg/MergeAppend.java

Lines 57 to 59 in 16fa673

    
           Preconditions.checkArgument( 
        
               manifest.snapshotId() == null || manifest.snapshotId() == -1, 
        
               "Snapshot id must be assigned during commit");

Instead, we set it to -1, and the SnapshotProducer will set the SnapshotID:

iceberg/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java

Lines 326 to 328 in 16fa673

    
           // the manifest must be rewritten with this update's snapshot ID and null first_row_ids 
        
           ManifestFile copiedManifest = copyManifest(manifest); 
        
           rewrittenAppendManifests.add(copiedManifest);

@amogh-jahagirdar Thanks for the review, I've added some tests 👍. I have to probe the Avro files directly because Java does allow for V1 tables to have Snapshot Inheritance (which is not allowed by the spec because snapshot_id has to be required).

@nastra Do you have time to check out the JUnit conventions? I'm not doing Java often enough :D

nastra · 2025-09-23T08:21:18Z

+    sql("INSERT INTO TABLE %s (id, data) VALUES (1, 'a'), (2, 'b')", SOURCE_NAME);
+    List<Object[]> result =
+        sql(
+            "CALL %s.system.snapshot(source_table => '%s', table => '%s', parallelism => %d, properties => map('format-version', '1'))",


I think it makes sense to just create two separate test methods without having any parallelism flag. Because what we're effectively testing is that the manifest has the correct format version. Having parallelism in the mix makes readers of the code wonder whether that flag is important or not to reproduce the issue

This will add an aditional test, but that works for me 👍

amogh-jahagirdar

Thanks @Fokko this looks great to me! Thank you @kevinjqliu @huaxingao @nastra for reviewing

kevinjqliu · 2025-09-23T15:18:10Z

do we want to backport this to Spark 3.5?

amogh-jahagirdar · 2025-09-23T15:31:30Z

@kevinjqliu Yes I think we should considering 3.5 is still going to be supported in the next release.

Fokko · 2025-09-23T16:04:23Z

@kevinjqliu @amogh-jahagirdar I think it makes sense to backport it to 3.4 and 3.5 for the 1.10.1 release since that one still supports both versions. Let me raise a PR in a sec

…gration actions (apache#14163)

…gration actions (#14163) (#14173)

…gration actions (apache#14163)

github-actions Bot added the spark label Sep 22, 2025

Spark: Pass format-version when creating a snapshot

651accb

Fokko force-pushed the fd-fix-snapshot branch from b5d0040 to 651accb Compare September 22, 2025 22:05

kevinjqliu approved these changes Sep 23, 2025

View reviewed changes

amogh-jahagirdar changed the title ~~Spark: Pass format-version when creating a snapshot~~ Spark: Pass format-version when creating a snapshot in table migration actions Sep 23, 2025

amogh-jahagirdar reviewed Sep 23, 2025

View reviewed changes

Throw in some tests and let them pass

fbfda2c

nastra reviewed Sep 23, 2025

View reviewed changes

Comment thread ...extensions/src/test/java/org/apache/iceberg/spark/extensions/TestSnapshotTableProcedure.java Outdated

nastra reviewed Sep 23, 2025

View reviewed changes

Comment thread ...extensions/src/test/java/org/apache/iceberg/spark/extensions/TestSnapshotTableProcedure.java Outdated

nastra reviewed Sep 23, 2025

View reviewed changes

Comment thread ...extensions/src/test/java/org/apache/iceberg/spark/extensions/TestSnapshotTableProcedure.java Outdated

nastra approved these changes Sep 23, 2025

View reviewed changes

Thanks Eduard!

54b3018

Fokko added this to the Iceberg 1.10.1 milestone Sep 23, 2025

manuzhang changed the title ~~Spark: Pass format-version when creating a snapshot in table migration actions~~ Spark 4.0: Pass format-version when creating a snapshot in table migration actions Sep 23, 2025

huaxingao approved these changes Sep 23, 2025

View reviewed changes

amogh-jahagirdar approved these changes Sep 23, 2025

View reviewed changes

amogh-jahagirdar merged commit a71f521 into apache:main Sep 23, 2025
27 checks passed

Fokko deleted the fd-fix-snapshot branch September 23, 2025 15:56

This was referenced Sep 23, 2025

Spark 3.5: Pass format-version when creating a snapshot #14169

Merged

Spark 3.4: Pass format-version when creating a snapshot #14170

Merged

Fokko added a commit to Fokko/iceberg that referenced this pull request Sep 23, 2025

Spark 4.0: Pass format-version when creating a snapshot in table mi…

92556dd

…gration actions (apache#14163)

huaxingao pushed a commit that referenced this pull request Sep 23, 2025

Spark 4.0: Pass format-version when creating a snapshot in table mi…

10ea261

…gration actions (#14163) (#14173)

gabeiglio pushed a commit to gabeiglio/iceberg that referenced this pull request Oct 1, 2025

Spark 4.0: Pass format-version when creating a snapshot in table mi…

2da1253

…gration actions (apache#14163)

manuzhang mentioned this pull request Oct 19, 2025

Tables with files imported to Iceberg using add_files contain inconsistent format-version manifest fields when using format-version 2 #13667

Closed

3 tasks

thomaschow pushed a commit to thomaschow/iceberg that referenced this pull request Jan 19, 2026

Spark 4.0: Pass format-version when creating a snapshot in table mi…

44e149f

…gration actions (apache#14163)

Uh oh!

Conversation

Fokko commented Sep 22, 2025

Uh oh!

kevinjqliu commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

Fokko commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nastra Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Fokko Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinjqliu commented Sep 23, 2025

Uh oh!

amogh-jahagirdar commented Sep 23, 2025

Uh oh!

Fokko commented Sep 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kevinjqliu commented Sep 23, 2025 •

edited

Loading

Fokko commented Sep 23, 2025 •

edited

Loading

Fokko Sep 23, 2025 •

edited

Loading