Skip to content

Conversation

@yuboxx
Copy link
Contributor

@yuboxx yuboxx commented Sep 25, 2025

Summary

  • The logic in BaseReplacePartitions.apply() doesn't run unpartitioned table check correctly. It simply counts for the fields in the partition spec without checking if they have void transform applied. This leads to the wrong behavior when insert overwrite runs on a table with partition columns removed, where it's supposed to replace the entire partitions instead of overwriting the partitions affected. The expected behavior is described in the public doc.

Change

  • Modified the check to use the method in PartitionSpec, which includes the void transform check.

Test Plan

  • Tested locally using Iceberg 1.4 and Spark 3.5.

@github-actions github-actions bot added the core label Sep 25, 2025
@huaxingao
Copy link
Contributor

@yuboxx Could you add a test please?

@yuboxx
Copy link
Contributor Author

yuboxx commented Sep 26, 2025

@yuboxx Could you add a test please?

Sure thing, could you take another look?

}

@TestTemplate
public void testReplaceAllVoidUnpartitionedTable() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throws IOException is redundant.

Comment on lines 237 to 246
assertThat(latestSnapshot(replaceMetadata, branch).allManifests(tableVoid.io())).hasSize(2);

validateManifestEntries(
latestSnapshot(replaceMetadata, branch).allManifests(tableVoid.io()).get(0),
ids(replaceId),
files(FILE_ALL_VOID_UNPARTITIONED_B),
statuses(Status.ADDED));

validateManifestEntries(
latestSnapshot(replaceMetadata, branch).allManifests(tableVoid.io()).get(1),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could extract a variable for latestSnapshot(replaceMetadata, branch).allManifests(tableVoid.io())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, updated!

Copy link
Contributor

@huaxingao huaxingao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yuboxx
Copy link
Contributor Author

yuboxx commented Sep 30, 2025

@huaxingao @ebyhr @singhpk234 Appreciate the quick turnaround, let me know if there is any steps I shall take to get this merged!

Copy link
Contributor

@singhpk234 singhpk234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too ! thanks @yuboxx !

let me know if there is any steps I shall take to get this merged

IMHO, sufficient community members have approved the change, lets wait for today (in case some one has more feedbacks), I will help merging this first thing in the morning unless someone beats me to it :)

@huaxingao (as you are RM) do you wanna consider this for 1.10.1 ?

@singhpk234 singhpk234 merged commit 00d18e3 into apache:main Sep 30, 2025
42 checks passed
@singhpk234
Copy link
Contributor

Thanks @yuboxx for change ! Thanks @huaxingao @ebyhr for the reviews.

@huaxingao
Copy link
Contributor

@singhpk234 Yes, I think this should be included in 1.10.1.

@huaxingao huaxingao added this to the Iceberg 1.10.1 milestone Sep 30, 2025
gabeiglio pushed a commit to gabeiglio/iceberg that referenced this pull request Oct 1, 2025
adawrapub pushed a commit to adawrapub/iceberg that referenced this pull request Oct 16, 2025
huaxingao pushed a commit to huaxingao/iceberg that referenced this pull request Nov 2, 2025
huaxingao pushed a commit that referenced this pull request Nov 3, 2025
thomaschow pushed a commit to thomaschow/iceberg that referenced this pull request Jan 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants