Spark 4.2: Apply session snapshot properties to maintenance actions by anuragmantri · Pull Request #18406 · apache/iceberg

anuragmantri · 2026-10-07T00:01:55Z

SQL users can tag write snapshots with spark.sql.iceberg.snapshot-property.* (#14545), but snapshots from rewrite_data_files, rewrite_position_delete_files, and rewrite_manifests ignore it, and procedures have no other way to set snapshot properties. This applies the session properties in BaseSnapshotUpdateSparkAction, through a SparkUtil helper shared with SparkWriteConf. Properties set through snapshotProperty() take precedence, as write options do for writes, and now also reach the dangling deletes commit that rewrite_data_files makes. Sessions that already set these properties for writes will now also tag maintenance snapshots.

Takes over #15842 (credit @puchengy) and adds the commit()-path test @anoopj asked for, plus the missing docs entry.

Test plan:

Rewrite data files and rewrite manifests each pick up session properties (covering both commit paths)
An explicit property overrides the session value;
CALL rewrite_manifests with the session property set tags the snapshot.
An explicit property reaches the dangling deletes commit.

AI Disclosure

Model: Claude Opus 5.5
Platform/Tool: Claude Code
Human Oversight: Fully reviewed by me.
Prompt Summary: Take over Spark: Support session-level snapshot properties for actions #15842 for Spark 4.2: apply session-config snapshot properties to snapshot-producing Spark actions, close the commit()-path test gap, and document the session property.

Co-authored-by: Pucheng Yang <pyang@pinterest.com>

anuragmantri · 2026-10-07T00:03:00Z

@mxm @dramaticlly - Could you please provide a review?

Also tagging @anoopj who reviewed #15842

mxm

Thanks @anuragmantri! Looks good to me. Just one question.

mxm · 2026-10-07T14:58:33Z

+    summary.putAll(
+        PropertyUtil.propertiesWithPrefix(
+            JavaConverters.mapAsJavaMap(spark.conf().getAll()),
+            SparkSQLProperties.SNAPSHOT_PROPERTY_PREFIX));


Is this the only place where we need to apply those?

For Spark actions, yes. BaseSnapshotUpdateSparkAction is the base class of all four actions that commit snapshots. RewriteDataFilesSparkAction, RewritePositionDeleteFilesSparkAction, and RemoveDanglingDeletesSparkAction commit through commitSummary(), and RewriteManifestsSparkAction commits through commit(). The tests cover both paths.

A few other places commit snapshots without picking this up:

add_files, migrate, and snapshot append through SparkTableUtil.

cherrypick_snapshot and publish_changes go through ManageSnapshots, which has no way to set summary properties, so they would need an API change first.

Metadata-only DELETE is a write-path gap, tracked in DELETE operations don't apply custom snapshot properties from session config #15060.

I'd like to keep this PR to the actions and handle SparkTableUtil in a follow-up.

on a tangentially related note, I think we might drop the snapshotProperty on RevemoDanglingDeletesSparkAction before but not anymore after this change.

SparkActions.get(spark).rewriteDataFiles(table) .option(RewriteDataFiles.REMOVE_DANGLING_DELETES, "true") .snapshotProperty("audit-id", "A") .execute();

also I think this duplicates with SparkWriteConf to retrieve the session level snapshot properties, might consider a common method in SparkUtil?

public static Map<String, String> sessionSnapshotProperties(SparkSession spark) { return PropertyUtil.propertiesWithPrefix( JavaConverters.mapAsJavaMap(spark.conf().getAll()), SparkSQLProperties.SNAPSHOT_PROPERTY_PREFIX); }

Good catch on the dangling deletes commit. Explicit properties were still dropped there, because only session properties reached the nested action. That also meant an explicit value won on the rewrite commit while the session value won on the dangling deletes commit.

I also moved the session lookup into SparkUtil.sessionSnapshotProperties, which BaseSnapshotUpdateSparkAction and SparkWriteConf now share, as you suggested.

dramaticlly · 2026-10-07T23:10:24Z

    assertThat(table.currentSnapshot().summary()).containsKeys(commitMetricsKeys);
  }

+  @TestTemplate


can we add a test to cover if override session or explicit snapshot property collide with reserved added-data-files, just to pin down the behavior.

Thanks for raising this. I tried it, and a colliding key fails the commit today with IllegalArgumentException: Multiple entries with same key: added-data-files=1 and added-data-files=999. The rewrite work is thrown away and the table is left unchanged.

That error comes from SnapshotSummary.Builder in core, and it happens the same way through snapshotProperty() and through writes with this session property, so this PR doesn't change it. #17009 tracks the reserved-key problem in core, and #17107 tried to fix it there before it went stale.

I'd rather not pin the current Guava error in a Spark test, because it would break as soon as core fixes #17009. Would you suggest a test that only checks that the commit fails and the table is unchanged?

dramaticlly · 2026-10-07T23:18:46Z

+    summary.putAll(
+        PropertyUtil.propertiesWithPrefix(
+            JavaConverters.mapAsJavaMap(spark.conf().getAll()),
+            SparkSQLProperties.SNAPSHOT_PROPERTY_PREFIX));


on a tangentially related note, I think we might drop the snapshotProperty on RevemoDanglingDeletesSparkAction before but not anymore after this change.

SparkActions.get(spark).rewriteDataFiles(table) .option(RewriteDataFiles.REMOVE_DANGLING_DELETES, "true") .snapshotProperty("audit-id", "A") .execute();

dramaticlly · 2026-10-07T23:20:09Z

+    summary.putAll(
+        PropertyUtil.propertiesWithPrefix(
+            JavaConverters.mapAsJavaMap(spark.conf().getAll()),
+            SparkSQLProperties.SNAPSHOT_PROPERTY_PREFIX));


also I think this duplicates with SparkWriteConf to retrieve the session level snapshot properties, might consider a common method in SparkUtil?

public static Map<String, String> sessionSnapshotProperties(SparkSession spark) { return PropertyUtil.propertiesWithPrefix( JavaConverters.mapAsJavaMap(spark.conf().getAll()), SparkSQLProperties.SNAPSHOT_PROPERTY_PREFIX); }

mxm · 2026-10-08T15:14:13Z

+    RemoveDanglingDeletesSparkAction removeDanglingDeletesAction =
+        new RemoveDanglingDeletesSparkAction(spark(), table).toBranch(branch);
+    commitSummary().forEach(removeDanglingDeletesAction::snapshotProperty);


This looks error-prone. Could we pass commitSummary() through a RemoveDanglingDeletesSparkAction constructor?

Agreed, that's cleaner. I made this change.

dramaticlly · 2026-10-08T20:58:36Z

 | spark.sql.iceberg.executor-cache.max-entry-size        | 67108864 (64MB)                                                | Max size per cache entry (bytes)                                                                                                |
 | spark.sql.iceberg.executor-cache.max-total-size        | 134217728 (128MB)                                              | Max total executor cache size (bytes)                                                                                           |
 | spark.sql.iceberg.executor-cache.locality.enabled      | false                                                          | Enables locality-aware executor cache usage                                                                                     |
+| spark.sql.iceberg.snapshot-property._custom-key_       | null                                                           | Adds an entry with custom-key and corresponding value to the summary of snapshots committed by writes and by the `rewrite_data_files`, `rewrite_position_delete_files`, and `rewrite_manifests` procedures. Write options and properties set on an action take precedence |


noticed there's later section which also populate the custom metadata to a snapshot summary during a SQL execution CommitMetadata.withCommitProperties, but list to per SQL writes. I think this complements with spark rewrite actions, might consider move later to it sits together? Some example on how explicit override session conf can also be helpful IMO.

Thanks, good suggestion. I added a paragraph right after the CommitMetadata example that covers the session property, where it applies. Please take a look.

mxm

Thanks @anuragmantri!

mxm · 2026-10-09T11:41:20Z

  protected RemoveDanglingDeletesSparkAction(SparkSession spark, Table table) {
-    super(spark);
+    this(spark, table, ImmutableMap.of());
+  }


Should we remove this constructor?

Yes, removed.

dramaticlly · 2026-10-09T23:58:06Z

        RuntimeException.class);
 ```
+
+Snapshot properties can also be set for a whole session with `spark.sql.iceberg.snapshot-property._custom-key_`. They apply to writes and to the `rewrite_data_files`, `rewrite_position_delete_files`, and `rewrite_manifests` procedures. A `snapshot-property.` write option or `CommitMetadata` overrides a session property with the same key. When calling an action through the Java API, `snapshotProperty()` does too.


nit: maybe highlight that write option or explicit argument override the session property in general?

anuragmantri

Thanks @mxm and @dramaticlly! I pushed another change, which removes the extra constructor and adds the snapshot property docs next to CommitMetadata. @mxm, could you take another look, since this landed after your approval?

anuragmantri · 2026-10-09T23:49:49Z

  protected RemoveDanglingDeletesSparkAction(SparkSession spark, Table table) {
-    super(spark);
+    this(spark, table, ImmutableMap.of());
+  }


Yes, removed.

anuragmantri · 2026-10-09T23:49:50Z

 | spark.sql.iceberg.executor-cache.max-entry-size        | 67108864 (64MB)                                                | Max size per cache entry (bytes)                                                                                                |
 | spark.sql.iceberg.executor-cache.max-total-size        | 134217728 (128MB)                                              | Max total executor cache size (bytes)                                                                                           |
 | spark.sql.iceberg.executor-cache.locality.enabled      | false                                                          | Enables locality-aware executor cache usage                                                                                     |
+| spark.sql.iceberg.snapshot-property._custom-key_       | null                                                           | Adds an entry with custom-key and corresponding value to the summary of snapshots committed by writes and by the `rewrite_data_files`, `rewrite_position_delete_files`, and `rewrite_manifests` procedures. Write options and properties set on an action take precedence |


Thanks, good suggestion. I added a paragraph right after the CommitMetadata example that covers the session property, where it applies. Please take a look.

Spark 4.2: Apply session snapshot properties to actions and procedures

0b1d5db

Co-authored-by: Pucheng Yang <pyang@pinterest.com>

github-actions Bot added spark docs labels Oct 7, 2026

mxm reviewed Oct 7, 2026

View reviewed changes

dramaticlly reviewed Oct 7, 2026

View reviewed changes

Address review comments

e6977a5

mxm reviewed Oct 8, 2026

View reviewed changes

Pass dangling deletes snapshot properties through the constructor

8b8b849

dramaticlly reviewed Oct 8, 2026

View reviewed changes

mxm approved these changes Oct 9, 2026

View reviewed changes

dramaticlly approved these changes Oct 9, 2026

View reviewed changes

document session snapshot properties

4588620

anuragmantri force-pushed the spark-action-snapshot-properties-b0fc5a8c branch from c4fc2be to 4588620 Compare October 10, 2026 00:03

anuragmantri commented Oct 10, 2026

View reviewed changes

anuragmantri changed the title ~~Spark 4.2: Apply session snapshot properties to actions and procedures~~ Spark 4.2: Apply session snapshot properties to maintenance actions Oct 10, 2026

Conversation

anuragmantri commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anuragmantri commented Oct 7, 2026

Uh oh!

mxm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dramaticlly Oct 8, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

mxm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuragmantri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

anuragmantri commented Oct 7, 2026 •

edited

Loading

dramaticlly Oct 8, 2026 •

edited

Loading