szehon-ho commented on code in PR #17440:
URL: https://github.com/apache/iceberg/pull/17440#discussion_r3687664818
##########
core/src/test/java/org/apache/iceberg/TestEntriesMetadataTable.java:
##########
@@ -152,4 +154,73 @@ public void testEntriesTableWithDeleteManifests() {
.as("Should contain 1 delete file record")
.isEqualTo(1);
}
+
+ @TestTemplate
+ public void testNotEqualRetainsPositionDeleteManifest() {
+ assumeThat(formatVersion).as("Only V2+ tables support
deletes").isGreaterThanOrEqualTo(2);
+
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newRowDelta().addDeletes(fileADeletes()).commit();
+
+ String deleteManifestPath =
+
Iterables.getOnlyElement(table.currentSnapshot().deleteManifests(table.io())).path();
+
+ Expression filter =
+ Expressions.notEqual("data_file.content",
FileContent.EQUALITY_DELETES.id());
+
+ assertManifestPlanned(new ManifestEntriesTable(table), filter,
deleteManifestPath);
+ assertManifestPlanned(new AllEntriesTable(table), filter,
deleteManifestPath);
+ }
+
+ @TestTemplate
+ public void testNotInRetainsPositionDeleteManifest() {
+ assumeThat(formatVersion).as("Only V2+ tables support
deletes").isGreaterThanOrEqualTo(2);
+
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newRowDelta().addDeletes(fileADeletes()).commit();
+
+ String deleteManifestPath =
+
Iterables.getOnlyElement(table.currentSnapshot().deleteManifests(table.io())).path();
+
+ Expression filter = Expressions.notIn("data_file.content",
FileContent.EQUALITY_DELETES.id());
+
+ assertManifestPlanned(new ManifestEntriesTable(table), filter,
deleteManifestPath);
+ assertManifestPlanned(new AllEntriesTable(table), filter,
deleteManifestPath);
+ }
+
+ @TestTemplate
+ public void testNotEqualPrunesDataManifest() {
+ assumeThat(formatVersion).as("Only V2+ tables support
deletes").isGreaterThanOrEqualTo(2);
+
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newRowDelta().addDeletes(fileADeletes()).commit();
+
+ String dataManifestPath =
+
Iterables.getOnlyElement(table.currentSnapshot().dataManifests(table.io())).path();
+
+ Expression filter = Expressions.notEqual("data_file.content",
FileContent.DATA.id());
+
+ assertManifestNotPlanned(new ManifestEntriesTable(table), filter,
dataManifestPath);
Review Comment:
Minor inconsistency: this one checks only `ManifestEntriesTable`, while the
other two tests assert against both `ManifestEntriesTable` and
`AllEntriesTable`.
##########
core/src/test/java/org/apache/iceberg/TestEntriesMetadataTable.java:
##########
@@ -152,4 +154,73 @@ public void testEntriesTableWithDeleteManifests() {
.as("Should contain 1 delete file record")
.isEqualTo(1);
}
+
+ @TestTemplate
+ public void testNotEqualRetainsPositionDeleteManifest() {
+ assumeThat(formatVersion).as("Only V2+ tables support
deletes").isGreaterThanOrEqualTo(2);
+
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newRowDelta().addDeletes(fileADeletes()).commit();
+
+ String deleteManifestPath =
+
Iterables.getOnlyElement(table.currentSnapshot().deleteManifests(table.io())).path();
+
+ Expression filter =
+ Expressions.notEqual("data_file.content",
FileContent.EQUALITY_DELETES.id());
+
+ assertManifestPlanned(new ManifestEntriesTable(table), filter,
deleteManifestPath);
+ assertManifestPlanned(new AllEntriesTable(table), filter,
deleteManifestPath);
+ }
+
+ @TestTemplate
+ public void testNotInRetainsPositionDeleteManifest() {
+ assumeThat(formatVersion).as("Only V2+ tables support
deletes").isGreaterThanOrEqualTo(2);
+
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newRowDelta().addDeletes(fileADeletes()).commit();
+
+ String deleteManifestPath =
+
Iterables.getOnlyElement(table.currentSnapshot().deleteManifests(table.io())).path();
+
+ Expression filter = Expressions.notIn("data_file.content",
FileContent.EQUALITY_DELETES.id());
Review Comment:
This test doesn't exercise `notIn`. A single-literal `NOT_IN` is collapsed
into a `NOT_EQ` bound predicate by `UnboundPredicate.bindInOperation`, so this
routes through `notEq`/`containsOnlyFileContent` and is an exact duplicate of
`testNotEqualRetainsPositionDeleteManifest`.
That leaves the new `containsOnlyFileContents(Set)` with no coverage for the
behavior it changes — the two multi-literal cases in `TestMetadataTableScans`
(`notIn(1, 2)` and `notIn(0, 1, 2)`) give identical results under the old and
new code, so they pass either way. A discriminating case needs a set containing
exactly one of the two delete content ids, e.g. `notIn("data_file.content", 2,
3)` against a delete manifest: the old `anyMatch` prunes it, the new code
retains it.
##########
core/src/test/java/org/apache/iceberg/TestEntriesMetadataTable.java:
##########
@@ -152,4 +154,73 @@ public void testEntriesTableWithDeleteManifests() {
.as("Should contain 1 delete file record")
.isEqualTo(1);
}
+
+ @TestTemplate
+ public void testNotEqualRetainsPositionDeleteManifest() {
+ assumeThat(formatVersion).as("Only V2+ tables support
deletes").isGreaterThanOrEqualTo(2);
+
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newRowDelta().addDeletes(fileADeletes()).commit();
+
+ String deleteManifestPath =
+
Iterables.getOnlyElement(table.currentSnapshot().deleteManifests(table.io())).path();
+
+ Expression filter =
+ Expressions.notEqual("data_file.content",
FileContent.EQUALITY_DELETES.id());
+
+ assertManifestPlanned(new ManifestEntriesTable(table), filter,
deleteManifestPath);
+ assertManifestPlanned(new AllEntriesTable(table), filter,
deleteManifestPath);
+ }
+
+ @TestTemplate
+ public void testNotInRetainsPositionDeleteManifest() {
+ assumeThat(formatVersion).as("Only V2+ tables support
deletes").isGreaterThanOrEqualTo(2);
+
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newRowDelta().addDeletes(fileADeletes()).commit();
+
+ String deleteManifestPath =
+
Iterables.getOnlyElement(table.currentSnapshot().deleteManifests(table.io())).path();
+
+ Expression filter = Expressions.notIn("data_file.content",
FileContent.EQUALITY_DELETES.id());
+
+ assertManifestPlanned(new ManifestEntriesTable(table), filter,
deleteManifestPath);
+ assertManifestPlanned(new AllEntriesTable(table), filter,
deleteManifestPath);
+ }
+
+ @TestTemplate
+ public void testNotEqualPrunesDataManifest() {
+ assumeThat(formatVersion).as("Only V2+ tables support
deletes").isGreaterThanOrEqualTo(2);
+
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newRowDelta().addDeletes(fileADeletes()).commit();
+
+ String dataManifestPath =
+
Iterables.getOnlyElement(table.currentSnapshot().dataManifests(table.io())).path();
+
+ Expression filter = Expressions.notEqual("data_file.content",
FileContent.DATA.id());
+
+ assertManifestNotPlanned(new ManifestEntriesTable(table), filter,
dataManifestPath);
+ }
+
+ private void assertManifestPlanned(Table metadataTable, Expression filter,
String manifestPath) {
Review Comment:
These two helpers reimplement `scannedPaths(TableScan)`, which
`TestMetadataTableScans` already inherits from `MetadataTableScanTestBase`
(they also do a redundant double copy:
`ImmutableList.copyOf(...).stream().collect(toImmutableList())`).
Since `TestMetadataTableScans` already owns
`testEntriesTableDataFileContentEq`/`NotEq`/`In`/`NotIn`, extending those with
the equality-delete cases would reuse the helper, get the mixed delete manifest
for free, and keep all `data_file.content` coverage in one place.
##########
core/src/test/java/org/apache/iceberg/TestEntriesMetadataTable.java:
##########
@@ -152,4 +154,73 @@ public void testEntriesTableWithDeleteManifests() {
.as("Should contain 1 delete file record")
.isEqualTo(1);
}
+
+ @TestTemplate
+ public void testNotEqualRetainsPositionDeleteManifest() {
Review Comment:
Both retention tests create a manifest holding only a position delete (or a
DV on v3) and assert at the manifest-planning level, so the mixed
position+equality manifest that motivates the fix is never actually built, and
the user-visible symptom is never shown.
`TestMetadataTableScans.preparePartitionedTable()` already produces exactly
that manifest — one `newRowDelta` adding `fileADeletes()`/`fileBDeletes()`
alongside `FILE_C2_DELETES`/`FILE_D2_DELETES`. Against that setup,
`notEqual("data_file.content", 1)` silently drops two real equality-delete rows
from the `entries` output before this fix and returns them after. That's the
strongest regression test available here and it costs one method.
##########
core/src/main/java/org/apache/iceberg/BaseEntriesTable.java:
##########
@@ -255,15 +254,31 @@ private <T> boolean fileContent(BoundReference<T> ref) {
return ref.fieldId() == DataFile.CONTENT.fieldId();
}
- private boolean contentMatch(Integer fileContentId) {
- if (FileContent.DATA.id() == fileContentId) {
- return ManifestContent.DATA.id() == manifestContentId;
- } else if (FileContent.EQUALITY_DELETES.id() == fileContentId
- || FileContent.POSITION_DELETES.id() == fileContentId) {
- return ManifestContent.DELETES.id() == manifestContentId;
- } else {
- return false;
+ private boolean mayContainFileContent(int fileContentId) {
+ if (ManifestContent.DATA == manifestContent) {
+ return FileContent.DATA.id() == fileContentId;
+ } else if (ManifestContent.DELETES == manifestContent) {
+ return FileContent.POSITION_DELETES.id() == fileContentId
+ || FileContent.EQUALITY_DELETES.id() == fileContentId;
+ }
+
+ // Be conservative for an unknown manifest content.
Review Comment:
This branch is unreachable — `ManifestContent` is a closed enum of `DATA`
and `DELETES`, and `ManifestFile.content()` never returns null. A `switch` on
the enum with the safe value in `default` would keep the same conservatism
while letting the compiler flag a newly added constant; as written, all three
helpers need a hand-audited fallback. Low stakes either way, since `true` for
may-contain and `false` for contains-only are both the safe directions.
--
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]
