Core: Fix partition info display in RewritePositionDeleteFilesSparkAction by manuzhang · Pull Request #15175 · apache/iceberg

manuzhang · 2026-01-29T02:50:12Z

After Fix:

Copilot

Pull request overview

This PR fixes an issue where partition information was not properly displayed in job descriptions for the RewritePositionDeleteFilesSparkAction in Spark 4.1. The fix adds a description field to StructProjection to carry partition information through the projection process.

Changes:

Modified StructProjection to store and return a description string
Updated PartitionUtil.coercePartition() to pass partition description when wrapping PartitionData
Modified RewritePositionDeleteFilesSparkAction to extract and use the partition description in job descriptions

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
api/src/main/java/org/apache/iceberg/util/StructProjection.java	Added description field and getter method, plus overloaded wrap method to accept description
core/src/main/java/org/apache/iceberg/util/PartitionUtil.java	Updated to pass partition description when wrapping PartitionData instances
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewritePositionDeleteFilesSparkAction.java	Added helper method to extract partition description for job descriptions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

nastra · 2026-03-03T09:59:10Z

@@ -377,4 +378,8 @@ private String jobDesc(
 table.name());
 }
 }
+
+ private String partitionDescription(StructLike struct) {
+ return struct instanceof StructProjection ? ((StructProjection) struct).description() : "";


we don't really need to cast with JDK 17 anymore. we can define the variable name already as part of the instanceof

nastra · 2026-03-04T10:33:52Z

@@ -362,7 +363,7 @@ private String jobDesc(
 runner.description(),
 group.info().globalIndex(),
 plan.totalGroupCount(),
- partition,
+partitionDescription(partition),


I think we have the same issue in RewriteDataFilesSparkAction?

I don't know why, but RewriteDataFilesSparkAction doesn't call coercePartition.

this is the places that I'm referring to:
iceberg/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java
Line 446 in b4ef17c
partition,

Without coercePartition, the partition info can be displayed in job description.

ah I see, got it, thanks. Nevertheless, I think we should still aim for implementing a proper toString() method

nastra · 2026-03-04T10:42:16Z

+
 public StructProjection copyFor(StructLike newStruct) {
- return new StructProjection(type, positionMap, nestedProjections).wrap(newStruct);
+ return new StructProjection(type, positionMap, nestedProjections).wrap(newStruct, description);


thinking more about this, I'm not sure that's the right approach going forward. Maybe it's better to implement toString and defer to the underlying struct in the toString implementation.
@Override public String toString() { return null != struct ? struct.toString() : null; }
that way we don't need any of the other changes and we can also make sure that this is properly handled not only for RewritePositionDeleteFilesAction but also for the action for rewriting data files

Do you mean like https://github.com/apache/iceberg/pull/13251/changes?

yes, like in that PR, although @stevenzwu has a point in that it should only include the projected fields

manuzhang · 2026-03-20T06:40:10Z

@nastra Please take another look. I followed the handling of coerced PartitionData in https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java#L510 and no other changes are needed now.

github-actions · 2026-04-23T00:35:29Z

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 dev@iceberg.apache.org list. Thank you for your contributions.

manuzhang · 2026-05-15T01:42:19Z

@nastra @huaxingao gentle ping for review. Thanks!

…writePositionDeleteFilesSparkAction

manuzhang · 2026-09-02T00:12:59Z

@huaxingao Could you please take a look at this PR? Many thanks!

huaxingao · 2026-09-02T00:59:12Z

@manuzhang since the change is only in core, could you fix the PR title to be Core: ...? Could you also add a regression test?

huaxingao · 2026-09-02T01:08:20Z

@nastra this now copies the coerced value into a PartitionData for the map key, following the same pattern PartitionStatsHandler uses, instead of touching StructProjection. That fixes this callsite but leaves StructProjection.toString() unhelpful for other callers. OK to land as-is with a follow-up issue, or would you prefer the toString() fix folded in here?

Verify that position delete rewrite groups expose readable partition descriptions for job metadata. Generated-by: Codex

huaxingao · 2026-09-02T21:33:02Z

@nastra Following up on my earlier question — I saw that the toString() route was attempted in #13251 and closed unmerged over @stevenzwu's point that it should only include the projected fields. Since this version doesn't touch api/ at all, I'll approve for now. Happy to file a follow-up issue for StructProjection.toString() if you'd like it tracked

huaxingao

LGTM

manuzhang requested a review from Copilot January 29, 2026 02:50

github-actionsBot added API spark core labels Jan 29, 2026

CopilotAI reviewed Jan 29, 2026
View reviewed changes

Comment threadcore/src/main/java/org/apache/iceberg/util/PartitionUtil.java Outdated
Comment thread...rk/src/main/java/org/apache/iceberg/spark/actions/RewritePositionDeleteFilesSparkAction.java Outdated

manuzhang force-pushed the partition-desc branch 2 times, most recently from be1b165 to 5beb72dCompare January 29, 2026 02:55

manuzhang requested a review from Copilot January 29, 2026 02:55

CopilotAI reviewed Jan 29, 2026
View reviewed changes

Comment thread...rk/src/main/java/org/apache/iceberg/spark/actions/RewritePositionDeleteFilesSparkAction.java Outdated
Comment threadapi/src/main/java/org/apache/iceberg/util/StructProjection.java

manuzhang force-pushed the partition-desc branch from 5beb72d to e72feb1Compare January 29, 2026 02:58

manuzhang requested a review from Copilot January 29, 2026 02:58

CopilotAI reviewed Jan 29, 2026
View reviewed changes

Comment threadapi/src/main/java/org/apache/iceberg/util/StructProjection.java
Comment threadapi/src/main/java/org/apache/iceberg/util/StructProjection.java Outdated

manuzhang force-pushed the partition-desc branch 2 times, most recently from f85cd05 to 85a351fCompare January 29, 2026 03:08

manuzhang marked this pull request as ready for review January 29, 2026 15:09

manuzhang requested a review from stevenzwu February 3, 2026 09:04

manuzhang requested review from huaxingao and nastra March 2, 2026 03:36