Skip to content

Core: Fix pruning for negated all_manifests filters #17346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
szehon-ho merged 1 commit into from
Aug 1, 2026
Merged

Core: Fix pruning for negated all_manifests filters #17346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions core/src/main/java/org/apache/iceberg/AllManifestsTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ private static class SnapshotEvaluator {
private final Expression boundExpr;

private SnapshotEvaluator(Expression expr, Types.StructType structType, boolean caseSensitive) {
this.boundExpr = Binder.bind(structType, expr, caseSensitive);
this.boundExpr = Binder.bind(structType, Expressions.rewriteNot(expr), caseSensitive);
}

private boolean eval(Snapshot snapshot) {
Expand Down Expand Up @@ -335,11 +335,6 @@ public Boolean alwaysFalse() {
return ROWS_CANNOT_MATCH;
}

@Override
public Boolean not(Boolean result) {
return !result;
}

@Override
public Boolean and(Boolean leftResult, Boolean rightResult) {
return leftResult && rightResult;
Expand Down
123 changes: 123 additions & 0 deletions core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,129 @@ public void testAllManifestsTableHonorsIgnoreResiduals() throws IOException {
validateTaskScanResiduals(scan2, true);
}

@TestTemplate
public void testAllManifestsTableNegatedPredicateOnNonSnapshotColumn() {
// Snapshots 1,2,3,4
preparePartitionedTableData();

Table allManifestsTable = new AllManifestsTable(table);
TableScan scan =
allManifestsTable
.newScan()
.filter(Expressions.not(Expressions.equal("content", ManifestContent.DATA.id())));

assertThat(scannedPaths(scan))
.as("A negated predicate on content must not prune snapshots")
.isEqualTo(expectedManifestListPaths(table.snapshots(), 1L, 2L, 3L, 4L));
}

@TestTemplate
public void testAllManifestsTableNotEqualPredicateOnNonSnapshotColumn() {
// Snapshots 1,2,3,4
preparePartitionedTableData();

Table allManifestsTable = new AllManifestsTable(table);
TableScan scan =
allManifestsTable
.newScan()
.filter(Expressions.notEqual("content", ManifestContent.DATA.id()));

assertThat(scannedPaths(scan))
.as("A not-equal predicate on content must not prune snapshots")
.isEqualTo(expectedManifestListPaths(table.snapshots(), 1L, 2L, 3L, 4L));
}

@TestTemplate
public void testAllManifestsTableNegatedOrPredicate() {
// Snapshots 1,2,3,4
preparePartitionedTableData();

long firstSnapshotId = 1L;

Table allManifestsTable = new AllManifestsTable(table);
TableScan scan =
allManifestsTable
.newScan()
.filter(
Expressions.not(
Expressions.or(
Expressions.equal("reference_snapshot_id", firstSnapshotId),
Expressions.equal("content", ManifestContent.DELETES.id()))));

/*
* NOT(reference_snapshot_id = firstSnapshotId OR content = DELETES)
*
* is rewritten as:
*
* reference_snapshot_id != firstSnapshotId
Comment on lines +520 to +524

@szehon-ho szehon-ho Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment explains the filter in terms of firstSnapshotId, but the code above uses the literal 1L, so there's no such name for a reader to map back to. Same thing in testAllManifestsTableNegatedAndPredicate below at lines 550, 554, and 557. Either spell the literal in the comment or introduce a firstSnapshotId local so the two agree.

@yangshangqing95 yangshangqing95 Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* AND content != DELETES
*
* The first snapshot can be pruned using reference_snapshot_id.
* The content predicate cannot prune the other snapshots because content
* is unknown while planning snapshot tasks.
*/
assertThat(scannedPaths(scan))
.as("Only the explicitly excluded snapshot should be pruned")
.isEqualTo(expectedManifestListPaths(table.snapshots(), 2L, 3L, 4L));
}

@TestTemplate
public void testAllManifestsTableNegatedAndPredicate() {
// Snapshots 1,2,3,4
preparePartitionedTableData();

long firstSnapshotId = 1L;

Table allManifestsTable = new AllManifestsTable(table);
TableScan scan =
allManifestsTable
.newScan()
.filter(
Expressions.not(
Expressions.and(
Expressions.equal("reference_snapshot_id", firstSnapshotId),
Expressions.equal("content", ManifestContent.DELETES.id()))));

/*
* NOT(reference_snapshot_id = firstSnapshotId AND content = DELETES)
*
* is rewritten as:
*
* reference_snapshot_id != firstSnapshotId
* OR content != DELETES
*
* Even for firstSnapshotId, content != DELETES may match. Therefore,
* no snapshot can be safely pruned.
*/
assertThat(scannedPaths(scan))
.as("A possibly matching content predicate must keep all snapshots")
.isEqualTo(expectedManifestListPaths(table.snapshots(), 1L, 2L, 3L, 4L));
}

@TestTemplate
public void testAllManifestsTableNegatedSnapshotOnlyPredicate() {
// Snapshots 1,2,3,4
preparePartitionedTableData();

Table allManifestsTable = new AllManifestsTable(table);
TableScan scan =
allManifestsTable
.newScan()
.filter(
Expressions.not(
Expressions.or(
Expressions.equal("reference_snapshot_id", 1L),
Expressions.equal("reference_snapshot_id", 2L))));

/*
* This confirms that rewriteNot does not merely disable pruning.
* Both excluded snapshots must still be pruned.
*/
assertThat(scannedPaths(scan))
.as("Negated snapshot-only predicates should still prune snapshots")
.isEqualTo(expectedManifestListPaths(table.snapshots(), 3L, 4L));
}

@TestTemplate
public void testPartitionsTableScanNoFilter() {
preparePartitionedTable();
Expand Down
73 changes: 73 additions & 0 deletions ...park-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
import org.apache.spark.sql.Encoders;
import org.apache.spark.sql.Row;
import org.apache.spark.sql.RowFactory;
import org.apache.spark.sql.catalyst.analysis.NoSuchTableException;
import org.apache.spark.sql.catalyst.util.DateTimeUtils;
import org.apache.spark.sql.types.StructType;
import org.junit.jupiter.api.AfterEach;
Expand Down Expand Up @@ -206,6 +207,78 @@ public void testUnpartitionedTable() throws Exception {
TestHelpers.nonDerivedSchema(actualFilesDs), expectedFiles.get(1), actualFiles.get(1));
}

@TestTemplate
public void testAllManifestsTableWithNegatedContentFilters() throws NoSuchTableException {
sql(
"CREATE TABLE %s (id bigint, data string) USING iceberg TBLPROPERTIES"
+ "('format-version'='%s', 'write.delete.mode'='merge-on-read')",
tableName, formatVersion);

List<SimpleRecord> records =
Lists.newArrayList(
new SimpleRecord(1, "a"), new SimpleRecord(2, "b"), new SimpleRecord(3, "c"));

spark
.createDataset(records, Encoders.bean(SimpleRecord.class))
.coalesce(1)
.writeTo(tableName)
.append();

// Create a delete manifest.
sql("DELETE FROM %s WHERE id = 1", tableName);

List<Object[]> expectedDeleteManifests =
sql(
"SELECT content, path, reference_snapshot_id "
+ "FROM %s.all_manifests "
+ "WHERE content = 1 "
+ "ORDER BY path, reference_snapshot_id",
tableName);

assertThat(expectedDeleteManifests)
.as("Test setup should create at least one delete manifest")
.isNotEmpty();

List<Object[]> notEqualResults =
sql(
"SELECT content, path, reference_snapshot_id "
+ "FROM %s.all_manifests "
+ "WHERE content != 0 "
+ "ORDER BY path, reference_snapshot_id",
tableName);

assertEquals(
"content != 0 should return all delete manifests",
expectedDeleteManifests,
notEqualResults);

List<Object[]> alternativeNotEqualResults =
sql(
"SELECT content, path, reference_snapshot_id "
+ "FROM %s.all_manifests "
+ "WHERE content <> 0 "
+ "ORDER BY path, reference_snapshot_id",
tableName);

assertEquals(
"content <> 0 should return all delete manifests",
expectedDeleteManifests,
alternativeNotEqualResults);

List<Object[]> explicitNotResults =
sql(
"SELECT content, path, reference_snapshot_id "
+ "FROM %s.all_manifests "
+ "WHERE NOT(content = 0) "
+ "ORDER BY path, reference_snapshot_id",
tableName);

assertEquals(
"NOT(content = 0) should return all delete manifests",
expectedDeleteManifests,
explicitNotResults);
}

@TestTemplate
public void testPositionDeletesTable() throws Exception {
sql(
Expand Down
73 changes: 73 additions & 0 deletions ...park-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
import org.apache.spark.sql.Encoders;
import org.apache.spark.sql.Row;
import org.apache.spark.sql.RowFactory;
import org.apache.spark.sql.catalyst.analysis.NoSuchTableException;
import org.apache.spark.sql.catalyst.util.DateTimeUtils;
import org.apache.spark.sql.types.StructType;
import org.junit.jupiter.api.AfterEach;
Expand Down Expand Up @@ -206,6 +207,78 @@ public void testUnpartitionedTable() throws Exception {
TestHelpers.nonDerivedSchema(actualFilesDs), expectedFiles.get(1), actualFiles.get(1));
}

@TestTemplate
public void testAllManifestsTableWithNegatedContentFilters() throws NoSuchTableException {
sql(
"CREATE TABLE %s (id bigint, data string) USING iceberg TBLPROPERTIES"
+ "('format-version'='%s', 'write.delete.mode'='merge-on-read')",
tableName, formatVersion);

List<SimpleRecord> records =
Lists.newArrayList(
new SimpleRecord(1, "a"), new SimpleRecord(2, "b"), new SimpleRecord(3, "c"));

spark
.createDataset(records, Encoders.bean(SimpleRecord.class))
.coalesce(1)
.writeTo(tableName)
.append();

// Create a delete manifest.
sql("DELETE FROM %s WHERE id = 1", tableName);

List<Object[]> expectedDeleteManifests =
sql(
"SELECT content, path, reference_snapshot_id "
+ "FROM %s.all_manifests "
+ "WHERE content = 1 "
+ "ORDER BY path, reference_snapshot_id",
tableName);

assertThat(expectedDeleteManifests)
.as("Test setup should create at least one delete manifest")
.isNotEmpty();

List<Object[]> notEqualResults =
sql(
"SELECT content, path, reference_snapshot_id "
+ "FROM %s.all_manifests "
+ "WHERE content != 0 "
+ "ORDER BY path, reference_snapshot_id",
tableName);

assertEquals(
"content != 0 should return all delete manifests",
expectedDeleteManifests,
notEqualResults);

List<Object[]> alternativeNotEqualResults =
sql(
"SELECT content, path, reference_snapshot_id "
+ "FROM %s.all_manifests "
+ "WHERE content <> 0 "
+ "ORDER BY path, reference_snapshot_id",
tableName);

assertEquals(
"content <> 0 should return all delete manifests",
expectedDeleteManifests,
alternativeNotEqualResults);

List<Object[]> explicitNotResults =
sql(
"SELECT content, path, reference_snapshot_id "
+ "FROM %s.all_manifests "
+ "WHERE NOT(content = 0) "
+ "ORDER BY path, reference_snapshot_id",
tableName);

assertEquals(
"NOT(content = 0) should return all delete manifests",
expectedDeleteManifests,
explicitNotResults);
}

@TestTemplate
public void testPositionDeletesTable() throws Exception {
sql(
Expand Down
Loading
Loading