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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
