You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "dramaticlly (via GitHub)" <gi...@apache.org> on 2026/08/21 22:17:21 UTC
[PR] Core: Read delete manifests correctly during snapshot expiration [iceberg]
dramaticlly opened a new pull request, #17763:
URL: https://github.com/apache/iceberg/pull/17763
2 bug make it right:
1. FileCleanupStrategy#readManifests projects the manifest list without the content field (517) and GenericManifestFile defaults content to DATA and only assigns it when the field is projected, so every manifest handled by ReachableFileCleanup and IncrementalFileCleanup reports ManifestContent.DATA, including delete manifests.
2. ManifestFiles.readPath open reads data manifest and ManifestReader does not derive its schema from FileType.
Clean up of delete manifest as part of snapshot expiration is already covered today in https://github.com/apache/iceberg/blob/7f879b11366e17a676a03f15247a821751415529/core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java#L1040
--
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: Read delete manifests correctly during snapshot expiration [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #17763:
URL: https://github.com/apache/iceberg/pull/17763#discussion_r3834104522
##########
core/src/main/java/org/apache/iceberg/ManifestFiles.java:
##########
@@ -113,7 +113,7 @@ public static CacheMetricsReport contentCacheStats(FileIO io) {
public static CloseableIterable<String> readPaths(
ManifestFile manifest, FileIO io, Map<Integer, PartitionSpec> specsById) {
return CloseableIterable.transform(
- read(manifest, io, specsById).select(ImmutableList.of("file_path")).liveEntries(),
+ open(manifest, io, specsById).select(ImmutableList.of("file_path")).liveEntries(),
Review Comment:
read() only handles data manifests while open handle both data and delete manifests
##########
core/src/main/java/org/apache/iceberg/ManifestFiles.java:
##########
@@ -113,7 +113,7 @@ public static CacheMetricsReport contentCacheStats(FileIO io) {
public static CloseableIterable<String> readPaths(
ManifestFile manifest, FileIO io, Map<Integer, PartitionSpec> specsById) {
return CloseableIterable.transform(
- read(manifest, io, specsById).select(ImmutableList.of("file_path")).liveEntries(),
+ open(manifest, io, specsById).select(ImmutableList.of("file_path")).liveEntries(),
Review Comment:
read() only handles data manifests while open() handle both data and delete manifests
--
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: Read delete manifests correctly during snapshot expiration [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #17763:
URL: https://github.com/apache/iceberg/pull/17763#issuecomment-5458426891
thank you Szehon for the detailed review and test suggestion
--
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: Read delete manifests correctly during snapshot expiration [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #17763:
URL: https://github.com/apache/iceberg/pull/17763#issuecomment-5376172217
@amogh-jahagirdar @szehon-ho can you help take a look?
--
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: Read delete manifests correctly during snapshot expiration [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho merged PR #17763:
URL: https://github.com/apache/iceberg/pull/17763
--
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: Read delete manifests correctly during snapshot expiration [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #17763:
URL: https://github.com/apache/iceberg/pull/17763#discussion_r3859530181
##########
core/src/main/java/org/apache/iceberg/FileCleanupStrategy.java:
##########
@@ -84,6 +84,7 @@ public abstract void cleanFiles(
ManifestFile.schema(),
ImmutableSet.of(
ManifestFile.PATH.fieldId(),
+ ManifestFile.MANIFEST_CONTENT.fieldId(),
Review Comment:
Thanks @szehon-ho , added UT as suggested for coverage on `MANIFEST_PROJECTION`.
Unlike existing UT, we do need to create a package private ReachableFileCleanup and calling protected method to exercise the projection logic. Also improve the coverage on v3 and v4 format in TestRemoveSnapshots.
Credit to @dkranchii for identifying this problem first in #17745 and #17772 adds the V3 parameterization in TestRemoveSnapshots, I added him as coauthor in the rebased commit
--
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: Read delete manifests correctly during snapshot expiration [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #17763:
URL: https://github.com/apache/iceberg/pull/17763#discussion_r3858663190
##########
core/src/main/java/org/apache/iceberg/FileCleanupStrategy.java:
##########
@@ -84,6 +84,7 @@ public abstract void cleanFiles(
ManifestFile.schema(),
ImmutableSet.of(
ManifestFile.PATH.fieldId(),
+ ManifestFile.MANIFEST_CONTENT.fieldId(),
Review Comment:
Can we add a test that fails without this line? Reverting just this field while keeping the `readPaths` change leaves every test green: `open()` sees `DATA`, dispatches to `read()`, and reading `file_path` still works. Both cleanup paths read under `suppressFailureWhenFinished`, so a regression here shows up as a warning and leaked delete files rather than a failure.
Something like this in a new `TestFileCleanupStrategy extends TestBase`, which also covers DVs in v3 and Parquet manifests in v4:
```java
@TestTemplate
void readManifestsProjectsManifestContent() throws IOException {
assumeThat(formatVersion)
.as("Delete files are only supported in V2 and later")
.isGreaterThanOrEqualTo(2);
table.newAppend().appendFile(FILE_A).commit();
table.newRowDelta().addDeletes(fileADeletes()).commit();
Snapshot snapshot = table.currentSnapshot();
Map<String, ManifestContent> expected =
snapshot.allManifests(table.io()).stream()
.collect(Collectors.toMap(ManifestFile::path, ManifestFile::content));
assertThat(expected).containsValue(ManifestContent.DELETES);
FileCleanupStrategy cleanup = new ReachableFileCleanup(table.io(), null, null, null);
Map<String, ManifestContent> actual = Maps.newHashMap();
try (CloseableIterable<ManifestFile> manifests = cleanup.readManifests(snapshot)) {
for (ManifestFile manifest : manifests) {
actual.put(manifest.path(), manifest.content());
}
}
assertThat(actual).isEqualTo(expected);
}
```
Note the containers are reused, so `path()` and `content()` have to be read inside the loop rather than collecting the `ManifestFile` references.
--
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: Read delete manifests correctly during snapshot expiration [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #17763:
URL: https://github.com/apache/iceberg/pull/17763#discussion_r3858663190
##########
core/src/main/java/org/apache/iceberg/FileCleanupStrategy.java:
##########
@@ -84,6 +84,7 @@ public abstract void cleanFiles(
ManifestFile.schema(),
ImmutableSet.of(
ManifestFile.PATH.fieldId(),
+ ManifestFile.MANIFEST_CONTENT.fieldId(),
Review Comment:
Can we add a test that fails without this line? Reverting just this field while keeping the `readPaths` change leaves every test green: `open()` sees `DATA`, dispatches to `read()`, and reading `file_path` still works. Both cleanup paths read under `suppressFailureWhenFinished`, so a regression here shows up as a warning and leaked delete files rather than a failure.
Something like this, which also covers DVs in v3 and Parquet manifests in v4:
```java
@TestTemplate
void readManifestsProjectsManifestContent() throws IOException {
assumeThat(formatVersion)
.as("Delete files are only supported in V2 and later")
.isGreaterThanOrEqualTo(2);
table.newAppend().appendFile(FILE_A).commit();
table.newRowDelta().addDeletes(fileADeletes()).commit();
Snapshot snapshot = table.currentSnapshot();
Map<String, ManifestContent> expected =
snapshot.allManifests(table.io()).stream()
.collect(Collectors.toMap(ManifestFile::path, ManifestFile::content));
assertThat(expected).containsValue(ManifestContent.DELETES);
FileCleanupStrategy cleanup = new ReachableFileCleanup(table.io(), null, null, null);
Map<String, ManifestContent> actual = Maps.newHashMap();
try (CloseableIterable<ManifestFile> manifests = cleanup.readManifests(snapshot)) {
for (ManifestFile manifest : manifests) {
actual.put(manifest.path(), manifest.content());
}
}
assertThat(actual).isEqualTo(expected);
}
```
Note the containers are reused, so `path()` and `content()` have to be read inside the loop rather than collecting the `ManifestFile` references.
Or if the test catches the bug , feel free to ignore.
--
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: Read delete manifests correctly during snapshot expiration [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #17763:
URL: https://github.com/apache/iceberg/pull/17763#issuecomment-5458261893
Merged, thanks @dramaticlly !
--
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