Core, Spark: add snapshot properties for snapshot created by wap branches #13922

yingjianwu98 · 2025-08-26T04:27:50Z

As we transition to using WAP with branches, we’ve noticed a gap in debugging observability compared to the previous approach using wap.id. Specifically, it’s difficult to determine which snapshots were created by WAP branches.

This PR addresses the issue by setting snapshot properties on snapshots that are created by WAP branches, making it easier to trace their origin and improve overall observability.

add snapshot properties for wap created snapshots

ebyhr · 2025-08-27T05:11:11Z

.../v4.0/spark/src/test/java/org/apache/iceberg/spark/sql/TestPartitionedWritesToWapBranch.java

+              assertThat(
+                      table
+                          .snapshot(table.refs().get(BRANCH).snapshotId())
+                          .summary()
+                          .get(SnapshotSummary.WAP_BRANCH_PROP))
+                  .isEqualTo(BRANCH);


We should use AbstractMapAssert's methods for better failure messages:

assertThat(table.snapshot(table.refs().get(BRANCH).snapshotId()).summary()) .containsEntry(SnapshotSummary.WAP_BRANCH_PROP, BRANCH);

ebyhr · 2025-08-27T05:12:38Z

spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/sql/TestPartitionedWrites.java

+              assertThat(
+                      table
+                          .snapshot(table.refs().get("main").snapshotId())
+                          .summary()
+                          .get(SnapshotSummary.WAP_BRANCH_PROP))
+                  .isNull();


We should use AbstractMapAssert's methods for better failure messages:

assertThat(table.snapshot(table.refs().get("main").snapshotId()).summary()) .doesNotContainKey(SnapshotSummary.WAP_BRANCH_PROP);

ebyhr · 2025-08-27T05:15:04Z

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

  public static final String REPLACED_MANIFESTS_COUNT = "manifests-replaced";
  public static final String KEPT_MANIFESTS_COUNT = "manifests-kept";
  public static final String PROCESSED_MANIFEST_ENTRY_COUNT = "entries-processed";
+  public static final String WAP_BRANCH_PROP = "wap.branch";


How about moving this constant under STAGED_WAP_ID_PROP?

spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/source/SparkPositionDeltaWrite.java

spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/source/SparkWrite.java

yingjianwu98 · 2025-08-31T17:00:11Z

Thanks for the comments @ebyhr @huaxingao

I have addressed them and let me know what you think!

ebyhr · 2025-09-01T05:48:20Z

spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/sql/TestPartitionedWrites.java

+    assertThatCode(
+            () -> {
+              sql("INSERT INTO %s VALUES (4, 'd')", tableName);
+              Table table = validationCatalog.loadTable(tableIdent);
+              assertThat(table.snapshot(table.refs().get("main").snapshotId()).summary())
+                  .doesNotContainKey(SnapshotSummary.WAP_BRANCH_PROP);
+            })
+        .doesNotThrowAnyException();


Why do we need the outer assertThatCode()...doesNotThrowAnyException() method?
Same for TestPartitionedWritesToWapBranch.

yeah, not really needed. Remove the outer assertion.

huaxingao · 2025-09-02T16:08:18Z

@yingjianwu98 Could you add coverage for the SparkPositionDeltaWrite code path?

huaxingao · 2025-09-02T16:10:09Z

@RussellSpitzer Could you please take a look at this PR too when you have a moment? Thanks!

RussellSpitzer · 2025-09-02T17:20:17Z

core/src/main/java/org/apache/iceberg/util/WapUtil.java

+    if (wapEnabled) {
+      if (wapId != null) {
+        operation.set(SnapshotSummary.STAGED_WAP_ID_PROP, wapId);
+        operation.stageOnly();


Setting this to stageOnly sounds like it's outside the concept of setting properties to me

RussellSpitzer · 2025-09-02T17:23:10Z

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

        .parse();
  }

+  private String getWapBranch() {


nit: We don't use get in our methods for getters

RussellSpitzer · 2025-09-02T17:25:54Z

spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/source/SparkPositionDeltaWrite.java

-        // write-audit-publish is enabled for this table and job
-        // stage the changes without changing the current snapshot
-        operation.set(SnapshotSummary.STAGED_WAP_ID_PROP, wapId);
-        operation.stageOnly();


I mentioned this above, but i'm not comfortable extracting stageOnly here. That seems a bit different than "setWapProperties". Perhaps a rename may make this a bit more clear, but currently it feels like we are hiding an important state change on operation.

RussellSpitzer · 2025-09-02T17:31:33Z

I think my big comment here is I have no problem with introducing the wap.branch property, but I don't think we should couple the "staging" logic in the function which sets that property. I think if we isolate the change to just setting either wap.id or branch, we are all set.

-- Note we may also add to the Spec index
https://github.com/apache/iceberg/blob/main/format/spec.md#other-fields

RussellSpitzer · 2025-09-02T17:32:42Z

core/src/main/java/org/apache/iceberg/util/WapUtil.java

+      boolean wapEnabled,
+      String wapId,
+      String branch,
+      Predicate<String> isWapBranch) {


Why do we need a functional interface here?

I initially want to pass the SparkWriteConf but it will introduce an extra spark dependency in the util module.

Hmmm let's try to think of an alternative here? Maybe we just only pass through "branch" if it is a Wap branch?

So have wapId and wapBranch as parameters

Sure sounds good, and I also agree to separate out the stageOnly commit into a separate function. And the function only responsibility will just be setting the snapshot properties.

RussellSpitzer · 2025-09-02T17:35:15Z

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

+    return confParser.stringConf().sessionConf(SparkSQLProperties.WAP_BRANCH).parseOptional();
+  }
+
+  public boolean isWapBranch(String branchName) {


I'm not sure why we need this method? Aren't we just always returning true of the wapBranch is not null and matches the branch being written too?

ah, I am thinking just wap branches will have this snapshot properties but for other branches, we won't set the property.

yingjianwu98 · 2025-09-03T05:26:20Z

Thanks everyone for the review again.

I have updated the PR based on suggestions and refactor the code.
I have also added some more tests but not sure if this is the best place to add these tests.

github-actions · 2025-10-07T00:17:11Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions · 2025-10-15T00:17:19Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

add snapshot properties for snapshot created by wap branches

4caad69

add snapshot properties for wap created snapshots

github-actions bot added spark core labels Aug 26, 2025

ebyhr reviewed Aug 27, 2025

View reviewed changes

yingjianwu98 changed the title ~~add snapshot properties for snapshot created by wap branches~~ Core, Spark: add snapshot properties for snapshot created by wap branches Aug 27, 2025

Yingjian Wu added 2 commits August 28, 2025 10:39

address comments

1cf813d

fix format

67956c9

huaxingao reviewed Aug 31, 2025

View reviewed changes

spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/source/SparkPositionDeltaWrite.java Outdated Show resolved Hide resolved

huaxingao reviewed Aug 31, 2025

View reviewed changes

spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/source/SparkWrite.java Outdated Show resolved Hide resolved

address comments

38f4e57

ebyhr reviewed Sep 1, 2025

View reviewed changes

address comments

3a26060

RussellSpitzer reviewed Sep 2, 2025

View reviewed changes

Yingjian Wu added 3 commits September 2, 2025 19:17

address comments

457d77e

address comments

e559b24

address comments

962696f

yingjianwu98 requested review from RussellSpitzer, ebyhr and huaxingao September 6, 2025 05:18

github-actions bot added the stale label Oct 7, 2025

github-actions bot closed this Oct 15, 2025

yingjianwu98 mentioned this pull request Nov 17, 2025

Core: populate target branch in snapshot property #14605

Open

Core, Spark: add snapshot properties for snapshot created by wap branches #13922

Core, Spark: add snapshot properties for snapshot created by wap branches #13922

Uh oh!

Conversation

yingjianwu98 commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebyhr Aug 27, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yingjianwu98 commented Aug 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huaxingao commented Sep 2, 2025

Uh oh!

huaxingao commented Sep 2, 2025

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!

RussellSpitzer commented Sep 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yingjianwu98 Sep 2, 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!

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!

yingjianwu98 commented Sep 3, 2025

Uh oh!

github-actions bot commented Oct 7, 2025

Uh oh!

github-actions bot commented Oct 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yingjianwu98 commented Aug 26, 2025 •

edited

Loading

ebyhr Aug 27, 2025 •

edited

Loading

yingjianwu98 Sep 2, 2025 •

edited

Loading