You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Prathameshk2696 (via GitHub)" <gi...@apache.org> on 2026/10/23 07:48:41 UTC
[PR] Fix NPE when snapshot summary is null in IncrementalFileCleanup [iceberg]
Prathameshk2696 opened a new pull request, #18624:
URL: https://github.com/apache/iceberg/pull/18624
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Handle null snapshot summary during incremental cleanup [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #18624:
URL: https://github.com/apache/iceberg/pull/18624#discussion_r4362986025
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1970,6 +2024,22 @@ private RemoveSnapshots removeSnapshots(Table table) {
return (RemoveSnapshots) removeSnapshots.withIncrementalCleanup(incrementalCleanup);
}
+ // simulates a snapshot with no summary, which is valid for snapshots written before summaries
+ // were introduced
+ private void dropSnapshotSummary(long snapshotId) throws IOException {
Review Comment:
this method name is inaccurate. it is more like `addSnapshotWithoutSummary`.
But for this trivial null check, I am not sure we need to add unit tests. does it impact the cleanup behavior or we just want to tolerate null summary?
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -97,11 +97,9 @@ public void cleanFiles(
Set<Long> pickedAncestorSnapshotIds = Sets.newHashSet();
for (long snapshotId : ancestorIds) {
+ Map<String, String> summary = beforeExpiration.snapshot(snapshotId).summary();
String sourceSnapshotId =
- beforeExpiration
- .snapshot(snapshotId)
- .summary()
- .get(SnapshotSummary.SOURCE_SNAPSHOT_ID_PROP);
+ summary != null ? summary.get(SnapshotSummary.SOURCE_SNAPSHOT_ID_PROP) : null;
Review Comment:
Use `PropertyUtil` here too, same as the expired-snapshot check below. `get` plus `Long.parseLong` is a second way to read this property.
```java
Long sourceSnapshotId =
summary != null
? PropertyUtil.propertyAsNullableLong(summary, SnapshotSummary.SOURCE_SNAPSHOT_ID_PROP)
: null;
if (sourceSnapshotId != null) {
pickedAncestorSnapshotIds.add(sourceSnapshotId);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Handle null snapshot summary during incremental cleanup [iceberg]
Posted by "Prathameshk2696 (via GitHub)" <gi...@apache.org>.
Prathameshk2696 commented on code in PR #18624:
URL: https://github.com/apache/iceberg/pull/18624#discussion_r4408857219
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -110,6 +113,57 @@ public void testExpireOlderThan() {
assertThat(deletedFiles).containsExactly(firstSnapshot.manifestListLocation());
}
+ @TestTemplate
+ public void expireOlderThanWithNullSummaryOnSurvivingAncestor() throws IOException {
+ assumeThat(incrementalCleanup).isTrue();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ Snapshot firstSnapshot = table.currentSnapshot();
+
+ waitUntilAfter(firstSnapshot.timestampMillis());
+
+ table.newAppend().appendFile(FILE_B).commit();
+ Snapshot secondSnapshot = table.currentSnapshot();
+ addSnapshotWithoutSummary(secondSnapshot.snapshotId());
+ assertThat(table.snapshot(secondSnapshot.snapshotId()).summary()).isNull();
+
+ long tAfterSecondCommit = waitUntilAfter(table.currentSnapshot().timestampMillis());
+
+ Set<String> deletedFiles = Sets.newHashSet();
+
+ removeSnapshots(table)
+ .expireOlderThan(tAfterSecondCommit)
Review Comment:
Dropped the tests as suggested in https://github.com/apache/iceberg/pull/18624#discussion_r4391729734.
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -110,6 +113,57 @@ public void testExpireOlderThan() {
assertThat(deletedFiles).containsExactly(firstSnapshot.manifestListLocation());
}
+ @TestTemplate
+ public void expireOlderThanWithNullSummaryOnSurvivingAncestor() throws IOException {
+ assumeThat(incrementalCleanup).isTrue();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ Snapshot firstSnapshot = table.currentSnapshot();
+
+ waitUntilAfter(firstSnapshot.timestampMillis());
+
+ table.newAppend().appendFile(FILE_B).commit();
+ Snapshot secondSnapshot = table.currentSnapshot();
+ addSnapshotWithoutSummary(secondSnapshot.snapshotId());
+ assertThat(table.snapshot(secondSnapshot.snapshotId()).summary()).isNull();
+
+ long tAfterSecondCommit = waitUntilAfter(table.currentSnapshot().timestampMillis());
+
+ Set<String> deletedFiles = Sets.newHashSet();
+
+ removeSnapshots(table)
+ .expireOlderThan(tAfterSecondCommit)
+ .deleteWith(deletedFiles::add)
+ .commit();
+
+ assertThat(table.snapshot(firstSnapshot.snapshotId())).isNull();
+ assertThat(table.snapshot(secondSnapshot.snapshotId())).isNotNull();
+ assertThat(deletedFiles).containsExactly(firstSnapshot.manifestListLocation());
+ }
+
+ @TestTemplate
+ public void expireOlderThanWithNullSummaryOnExpiredAncestor() throws IOException {
Review Comment:
Dropped the tests as suggested in https://github.com/apache/iceberg/pull/18624#discussion_r4391729734.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Handle null snapshot summary during incremental cleanup [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #18624:
URL: https://github.com/apache/iceberg/pull/18624#discussion_r4391642420
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1970,6 +2024,22 @@ private RemoveSnapshots removeSnapshots(Table table) {
return (RemoveSnapshots) removeSnapshots.withIncrementalCleanup(incrementalCleanup);
}
+ // simulates a snapshot with no summary, which is valid for snapshots written before summaries
+ // were introduced
+ private void addSnapshotWithoutSummary(long snapshotId) throws IOException {
Review Comment:
use JSON serialization round-trip to modify an existing snapshot seems hacky.
why don't we construct the Snapshot and TableMetadata directly via constructor or builder?
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -110,6 +113,57 @@ public void testExpireOlderThan() {
assertThat(deletedFiles).containsExactly(firstSnapshot.manifestListLocation());
}
+ @TestTemplate
+ public void expireOlderThanWithNullSummaryOnSurvivingAncestor() throws IOException {
+ assumeThat(incrementalCleanup).isTrue();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ Snapshot firstSnapshot = table.currentSnapshot();
+
+ waitUntilAfter(firstSnapshot.timestampMillis());
+
+ table.newAppend().appendFile(FILE_B).commit();
+ Snapshot secondSnapshot = table.currentSnapshot();
+ addSnapshotWithoutSummary(secondSnapshot.snapshotId());
+ assertThat(table.snapshot(secondSnapshot.snapshotId()).summary()).isNull();
+
+ long tAfterSecondCommit = waitUntilAfter(table.currentSnapshot().timestampMillis());
+
+ Set<String> deletedFiles = Sets.newHashSet();
+
+ removeSnapshots(table)
+ .expireOlderThan(tAfterSecondCommit)
+ .deleteWith(deletedFiles::add)
+ .commit();
+
+ assertThat(table.snapshot(firstSnapshot.snapshotId())).isNull();
+ assertThat(table.snapshot(secondSnapshot.snapshotId())).isNotNull();
+ assertThat(deletedFiles).containsExactly(firstSnapshot.manifestListLocation());
+ }
+
+ @TestTemplate
+ public void expireOlderThanWithNullSummaryOnExpiredAncestor() throws IOException {
Review Comment:
how is this test different with the one above?
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -110,6 +113,57 @@ public void testExpireOlderThan() {
assertThat(deletedFiles).containsExactly(firstSnapshot.manifestListLocation());
}
+ @TestTemplate
+ public void expireOlderThanWithNullSummaryOnSurvivingAncestor() throws IOException {
+ assumeThat(incrementalCleanup).isTrue();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ Snapshot firstSnapshot = table.currentSnapshot();
+
+ waitUntilAfter(firstSnapshot.timestampMillis());
+
+ table.newAppend().appendFile(FILE_B).commit();
+ Snapshot secondSnapshot = table.currentSnapshot();
+ addSnapshotWithoutSummary(secondSnapshot.snapshotId());
+ assertThat(table.snapshot(secondSnapshot.snapshotId()).summary()).isNull();
+
+ long tAfterSecondCommit = waitUntilAfter(table.currentSnapshot().timestampMillis());
+
+ Set<String> deletedFiles = Sets.newHashSet();
+
+ removeSnapshots(table)
+ .expireOlderThan(tAfterSecondCommit)
Review Comment:
this condition is a little weird. by time, the second snapshot should be expired. but I guess the second snapshot is retained because it is the only snapshot retained. is this the scenario we are targeting? if not, why don't expire a timestamp after the first snapshot?
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1970,6 +2024,22 @@ private RemoveSnapshots removeSnapshots(Table table) {
return (RemoveSnapshots) removeSnapshots.withIncrementalCleanup(incrementalCleanup);
}
+ // simulates a snapshot with no summary, which is valid for snapshots written before summaries
+ // were introduced
+ private void dropSnapshotSummary(long snapshotId) throws IOException {
Review Comment:
I would drop both tests and `addSnapshotWithoutSummary`.
The NPE stopped expiration, but a null summary does not change which files are deleted. Cleanup reads the summary only to find `source-snapshot-id`. With no summary, that id is missing, so the cherry-pick checks do not apply and expiration deletes the same files as a normal append. `testExpireOlderThan` already checks that. Both new tests check it again: the older snapshot is removed, and only its manifest list is deleted.
A test whose only new check is "expiration does not throw" needs a metadata rewrite to strip the summary. That setup is more than this null check needs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Handle null snapshot summary during incremental cleanup [iceberg]
Posted by "Prathameshk2696 (via GitHub)" <gi...@apache.org>.
Prathameshk2696 commented on PR #18624:
URL: https://github.com/apache/iceberg/pull/18624#issuecomment-6360323580
@stevenzwu @huaxingao Could you PTAL the NPE fix change here ? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Handle null snapshot summary during incremental cleanup [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu merged PR #18624:
URL: https://github.com/apache/iceberg/pull/18624
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Handle null snapshot summary during incremental cleanup [iceberg]
Posted by "Prathameshk2696 (via GitHub)" <gi...@apache.org>.
Prathameshk2696 commented on code in PR #18624:
URL: https://github.com/apache/iceberg/pull/18624#discussion_r4385343211
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1970,6 +2024,22 @@ private RemoveSnapshots removeSnapshots(Table table) {
return (RemoveSnapshots) removeSnapshots.withIncrementalCleanup(incrementalCleanup);
}
+ // simulates a snapshot with no summary, which is valid for snapshots written before summaries
+ // were introduced
+ private void dropSnapshotSummary(long snapshotId) throws IOException {
Review Comment:
Renamed the method to `addSnapshotWithoutSummary`.
@stevenzwu Should we remove test addition for null check if test addition is overkill? It did impact the cleanup behavior as IncrementalFileCleanup logic encountered NPE and failed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Handle null snapshot summary during incremental cleanup [iceberg]
Posted by "Prathameshk2696 (via GitHub)" <gi...@apache.org>.
Prathameshk2696 commented on code in PR #18624:
URL: https://github.com/apache/iceberg/pull/18624#discussion_r4385334720
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -97,11 +97,9 @@ public void cleanFiles(
Set<Long> pickedAncestorSnapshotIds = Sets.newHashSet();
for (long snapshotId : ancestorIds) {
+ Map<String, String> summary = beforeExpiration.snapshot(snapshotId).summary();
String sourceSnapshotId =
- beforeExpiration
- .snapshot(snapshotId)
- .summary()
- .get(SnapshotSummary.SOURCE_SNAPSHOT_ID_PROP);
+ summary != null ? summary.get(SnapshotSummary.SOURCE_SNAPSHOT_ID_PROP) : null;
Review Comment:
Yes, updated it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Handle null snapshot summary during incremental cleanup [iceberg]
Posted by "Prathameshk2696 (via GitHub)" <gi...@apache.org>.
Prathameshk2696 commented on code in PR #18624:
URL: https://github.com/apache/iceberg/pull/18624#discussion_r4408858456
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1970,6 +2024,22 @@ private RemoveSnapshots removeSnapshots(Table table) {
return (RemoveSnapshots) removeSnapshots.withIncrementalCleanup(incrementalCleanup);
}
+ // simulates a snapshot with no summary, which is valid for snapshots written before summaries
+ // were introduced
+ private void addSnapshotWithoutSummary(long snapshotId) throws IOException {
Review Comment:
Dropped the tests as suggested in https://github.com/apache/iceberg/pull/18624#discussion_r4391729734.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org