This is an automated email from the ASF dual-hosted git repository.
szehon-ho pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg.git
The following commit(s) were added to refs/heads/main by this push:
new 98041f0ecb Core: Fix incorrect delete manifest pruning in entries
metadata tables (#17440)
98041f0ecb is described below
commit 98041f0ecb9f01934a4e92a82b35ecd834b05aff
Author: yangshangqing <[email protected]>
AuthorDate: Fri Jul 31 23:00:27 2026 -0400
Core: Fix incorrect delete manifest pruning in entries metadata tables
(#17440)
---
.../java/org/apache/iceberg/BaseEntriesTable.java | 51 +++++++-----
.../apache/iceberg/TestEntriesMetadataTable.java | 29 +++++++
.../org/apache/iceberg/TestMetadataTableScans.java | 90 ++++++++++++++++++----
3 files changed, 137 insertions(+), 33 deletions(-)
diff --git a/core/src/main/java/org/apache/iceberg/BaseEntriesTable.java
b/core/src/main/java/org/apache/iceberg/BaseEntriesTable.java
index a4ee6eec32..d51a419dfe 100644
--- a/core/src/main/java/org/apache/iceberg/BaseEntriesTable.java
+++ b/core/src/main/java/org/apache/iceberg/BaseEntriesTable.java
@@ -117,13 +117,13 @@ abstract class BaseEntriesTable extends BaseMetadataTable
{
private class ManifestEvalVisitor extends
ExpressionVisitors.BoundExpressionVisitor<Boolean> {
- private int manifestContentId;
+ private ManifestContent manifestContent;
private static final boolean ROWS_MIGHT_MATCH = true;
private static final boolean ROWS_CANNOT_MATCH = false;
private boolean eval(ManifestFile manifestFile) {
- this.manifestContentId = manifestFile.content().id();
+ this.manifestContent = manifestFile.content();
return ExpressionVisitors.visitEvaluator(boundExpr, this);
}
@@ -204,7 +204,7 @@ abstract class BaseEntriesTable extends BaseMetadataTable {
public <T> Boolean eq(BoundReference<T> ref, Literal<T> lit) {
if (fileContent(ref)) {
Literal<Integer> intLit = lit.to(Types.IntegerType.get());
- if (!contentMatch(intLit.value())) {
+ if (!mayContainFileContent(intLit.value())) {
return ROWS_CANNOT_MATCH;
}
}
@@ -215,7 +215,7 @@ abstract class BaseEntriesTable extends BaseMetadataTable {
public <T> Boolean notEq(BoundReference<T> ref, Literal<T> lit) {
if (fileContent(ref)) {
Literal<Integer> intLit = lit.to(Types.IntegerType.get());
- if (contentMatch(intLit.value())) {
+ if (containsOnlyFileContent(intLit.value())) {
return ROWS_CANNOT_MATCH;
}
}
@@ -225,7 +225,7 @@ abstract class BaseEntriesTable extends BaseMetadataTable {
@Override
public <T> Boolean in(BoundReference<T> ref, Set<T> literalSet) {
if (fileContent(ref)) {
- if (literalSet.stream().noneMatch(lit -> contentMatch((Integer)
lit))) {
+ if (literalSet.stream().noneMatch(lit ->
mayContainFileContent((Integer) lit))) {
return ROWS_CANNOT_MATCH;
}
}
@@ -234,11 +234,10 @@ abstract class BaseEntriesTable extends BaseMetadataTable
{
@Override
public <T> Boolean notIn(BoundReference<T> ref, Set<T> literalSet) {
- if (fileContent(ref)) {
- if (literalSet.stream().anyMatch(lit -> contentMatch((Integer)
lit))) {
- return ROWS_CANNOT_MATCH;
- }
+ if (fileContent(ref) && containsAllPossibleFileContents(literalSet)) {
+ return ROWS_CANNOT_MATCH;
}
+
return ROWS_MIGHT_MATCH;
}
@@ -256,15 +255,31 @@ abstract class BaseEntriesTable extends BaseMetadataTable
{
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) {
+ return switch (manifestContent) {
+ case DATA -> FileContent.DATA.id() == fileContentId;
+ case DELETES ->
+ FileContent.POSITION_DELETES.id() == fileContentId
+ || FileContent.EQUALITY_DELETES.id() == fileContentId;
+ };
+ }
+
+ private boolean containsOnlyFileContent(int fileContentId) {
+ // A data manifest contains only data files(content=0), while a delete
manifest may contain
+ // both position(content=1) and equality(content=2) deletes.
+ return switch (manifestContent) {
+ case DATA -> FileContent.DATA.id() == fileContentId;
+ case DELETES -> false;
+ };
+ }
+
+ private <T> boolean containsAllPossibleFileContents(Set<T>
fileContentIds) {
+ return switch (manifestContent) {
+ case DATA -> fileContentIds.contains(FileContent.DATA.id());
+ case DELETES ->
+ fileContentIds.contains(FileContent.POSITION_DELETES.id())
+ &&
fileContentIds.contains(FileContent.EQUALITY_DELETES.id());
+ };
}
}
}
diff --git
a/core/src/test/java/org/apache/iceberg/TestEntriesMetadataTable.java
b/core/src/test/java/org/apache/iceberg/TestEntriesMetadataTable.java
index f80f29256d..132c2ef678 100644
--- a/core/src/test/java/org/apache/iceberg/TestEntriesMetadataTable.java
+++ b/core/src/test/java/org/apache/iceberg/TestEntriesMetadataTable.java
@@ -21,8 +21,11 @@ package org.apache.iceberg;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assumptions.assumeThat;
+import java.io.IOException;
import java.util.List;
+import org.apache.iceberg.expressions.Expression;
import org.apache.iceberg.expressions.Expressions;
+import org.apache.iceberg.io.CloseableIterable;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.types.TypeUtil;
@@ -187,4 +190,30 @@ public class TestEntriesMetadataTable extends TestBase {
.allSatisfy(task ->
assertThat(task.residual()).isEqualTo(Expressions.alwaysTrue()));
}
}
+
+ @TestTemplate
+ public void testInvalidContentPredicatesPruneAllManifests() throws
IOException {
+ assumeThat(formatVersion).as("Only V2+ tables support delete
files").isGreaterThanOrEqualTo(2);
+
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newRowDelta().addDeletes(fileADeletes()).commit();
+
+ List<Table> metadataTables =
+ ImmutableList.of(new ManifestEntriesTable(table), new
AllEntriesTable(table));
+
+ List<Expression> filters =
+ ImmutableList.of(
+ Expressions.equal("data_file.content", 3),
Expressions.in("data_file.content", 3, 4));
+
+ for (Table metadataTable : metadataTables) {
+ for (Expression filter : filters) {
+ try (CloseableIterable<FileScanTask> tasks =
+ metadataTable.newScan().filter(filter).planFiles()) {
+ assertThat(tasks)
+ .as("Should not plan manifests for %s with filter %s",
metadataTable.name(), filter)
+ .isEmpty();
+ }
+ }
+ }
+ }
}
diff --git a/core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
b/core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
index d5a0402a7a..75edd882d5 100644
--- a/core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
+++ b/core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
@@ -33,6 +33,7 @@ import java.util.concurrent.Executors;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;
+import org.apache.iceberg.expressions.Evaluator;
import org.apache.iceberg.expressions.Expression;
import org.apache.iceberg.expressions.Expressions;
import org.apache.iceberg.expressions.Literal;
@@ -251,23 +252,23 @@ public class TestMetadataTableScans extends
MetadataTableScanTestBase {
}
@TestTemplate
- public void testEntriesTableDateFileContentNotEq() {
- preparePartitionedTable();
+ public void testEntriesTableDataFileContentNotEq() {
+ // Write position and equality deletes into the same delete manifest.
+ preparePartitionedTable(true);
Table entriesTable = new ManifestEntriesTable(table);
Expression notData = Expressions.notEqual("data_file.content", 0);
- TableScan entriesTableScan = entriesTable.newScan().filter(notData);
- Set<String> expected =
+ Set<String> expectedDeleteManifestPaths =
table.currentSnapshot().deleteManifests(table.io()).stream()
.map(ManifestFile::path)
.collect(Collectors.toSet());
- assertThat(scannedPaths(entriesTableScan))
+ assertThat(scannedPaths(entriesTable.newScan().filter(notData)))
.as("Expected manifest filter by data file content does not match")
- .isEqualTo(expected);
+ .isEqualTo(expectedDeleteManifestPaths);
- Set<String> allManifests =
+ Set<String> allManifestPaths =
table.currentSnapshot().allManifests(table.io()).stream()
.map(ManifestFile::path)
.collect(Collectors.toSet());
@@ -275,7 +276,17 @@ public class TestMetadataTableScans extends
MetadataTableScanTestBase {
scannedPaths(
entriesTable.newScan().filter(Expressions.notEqual("data_file.content", 3))))
.as("Expected manifest filter by data file content does not match")
- .isEqualTo(allManifests);
+ .isEqualTo(allManifestPaths);
+
+ Expression notPositionDeletes = Expressions.notEqual("data_file.content",
1);
+ assertThat(scannedPaths(entriesTable.newScan().filter(notPositionDeletes)))
+ .as("Expected mixed delete manifest to be retained for content !=
POSITION_DELETES")
+ .isEqualTo(allManifestPaths);
+
+ Expression notEqualityDeletes = Expressions.notEqual("data_file.content",
2);
+ assertThat(scannedPaths(entriesTable.newScan().filter(notEqualityDeletes)))
+ .as("Expected mixed delete manifest to be retained for content !=
EQUALITY_DELETES")
+ .isEqualTo(allManifestPaths);
}
@TestTemplate
@@ -317,39 +328,51 @@ public class TestMetadataTableScans extends
MetadataTableScanTestBase {
@TestTemplate
public void testEntriesTableDataFileContentNotIn() {
- preparePartitionedTable();
+ // Write position and equality deletes into the same delete manifest.
+ preparePartitionedTable(true);
Table entriesTable = new ManifestEntriesTable(table);
Expression notIn0 = Expressions.notIn("data_file.content", 0);
TableScan scan1 = entriesTable.newScan().filter(notIn0);
- Set<String> expectedDeleteManifestPath =
+ Set<String> expectedDeleteManifestPaths =
table.currentSnapshot().deleteManifests(table.io()).stream()
.map(ManifestFile::path)
.collect(Collectors.toSet());
assertThat(scannedPaths(scan1))
.as("Expected manifest filter by data file content does not match")
- .isEqualTo(expectedDeleteManifestPath);
+ .isEqualTo(expectedDeleteManifestPaths);
Expression notIn12 = Expressions.notIn("data_file.content", 1, 2);
TableScan scan2 = entriesTable.newScan().filter(notIn12);
- Set<String> expectedDataManifestPath =
+ Set<String> expectedDataManifestPaths =
table.currentSnapshot().dataManifests(table.io()).stream()
.map(ManifestFile::path)
.collect(Collectors.toSet());
assertThat(scannedPaths(scan2))
.as("Expected manifest filter by data file content does not match")
- .isEqualTo(expectedDataManifestPath);
+ .isEqualTo(expectedDataManifestPaths);
Expression notInNeither = Expressions.notIn("data_file.content", 3);
- Set<String> allManifests = Sets.union(expectedDataManifestPath,
expectedDeleteManifestPath);
+ Set<String> allManifestPaths =
+ Sets.union(expectedDataManifestPaths, expectedDeleteManifestPaths);
assertThat(scannedPaths(entriesTable.newScan().filter(notInNeither)))
.as("Expected manifest filter by data file content does not match")
- .isEqualTo(allManifests);
+ .isEqualTo(allManifestPaths);
Expression notInAll = Expressions.notIn("data_file.content", 0, 1, 2);
assertThat(scannedPaths(entriesTable.newScan().filter(notInAll)))
.as("Expected manifest filter by data file content does not match")
.isEmpty();
+
+ Expression notInPositionDeleteAndInvalid =
Expressions.notIn("data_file.content", 1, 3);
+
assertThat(scannedPaths(entriesTable.newScan().filter(notInPositionDeleteAndInvalid)))
+ .as("Expected mixed delete manifest to be retained")
+ .isEqualTo(allManifestPaths);
+
+ Expression notInEqualityDeleteAndInvalid =
Expressions.notIn("data_file.content", 2, 3);
+
assertThat(scannedPaths(entriesTable.newScan().filter(notInEqualityDeleteAndInvalid)))
+ .as("Expected mixed delete manifest to be retained")
+ .isEqualTo(allManifestPaths);
}
@TestTemplate
@@ -1833,6 +1856,43 @@ public class TestMetadataTableScans extends
MetadataTableScanTestBase {
.isEqualTo(2);
}
+ @TestTemplate
+ public void testNotEqualReturnsEqualityDeletesFromMixedDeleteManifest()
throws IOException {
+ assumeThat(formatVersion).as("Only V2+ tables support
deletes").isGreaterThanOrEqualTo(2);
+
+ // Creates one delete manifest containing both position and equality
deletes.
+ preparePartitionedTable(true);
+
+ assertThat(table.currentSnapshot().deleteManifests(table.io())).hasSize(1);
+
+ Table entriesTable = new ManifestEntriesTable(table);
+ TableScan scan =
+ entriesTable
+ .newScan()
+ .select("data_file.content")
+ .filter(Expressions.notEqual("data_file.content", 1));
+
+ Accessor<StructLike> contentAccessor =
+ scan.schema().accessorForField(DataFile.CONTENT.fieldId());
+
+ List<Integer> actualContents = Lists.newArrayList();
+
+ try (CloseableIterable<FileScanTask> tasks = scan.planFiles()) {
+ for (FileScanTask task : tasks) {
+ Evaluator residualEvaluator = new Evaluator(scan.schema().asStruct(),
task.residual());
+ try (CloseableIterable<StructLike> rows = task.asDataTask().rows()) {
+ for (StructLike row : rows) {
+ if (residualEvaluator.eval(row)) {
+ actualContents.add((Integer) contentAccessor.get(row));
+ }
+ }
+ }
+ }
+ }
+
+ assertThat(actualContents).containsExactlyInAnyOrder(0, 0, 0, 0, 2, 2);
+ }
+
private int rowCount(TableScan scan) throws IOException {
int count = 0;
try (CloseableIterable<FileScanTask> tasks = scan.planFiles()) {
