szehon-ho commented on code in PR #17346:
URL: https://github.com/apache/iceberg/pull/17346#discussion_r3687686139
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -467,6 +467,125 @@ public void testAllManifestsTableHonorsIgnoreResiduals()
throws IOException {
validateTaskScanResiduals(scan2, true);
}
+ @TestTemplate
+ public void testAllManifestsTableNegatedPredicateOnNonSnapshotColumn()
throws IOException {
Review Comment:
Nit: these five new tests don't need `throws IOException`. Both
`preparePartitionedTableData()` and `scannedPaths()` are declared without
checked exceptions, so nothing in the body can throw it. The `throws
IOException` on the tests just above
(`testAllManifestsTableHonorsIgnoreResiduals` and friends) is real — those call
`validateTaskScanResiduals`, which does throw. The closest analogues to these
new tests are `testAllManifestsTableSnapshotGt`/`Lt`/`Eq`, which declare no
`throws` at all. Applies to all five: lines 471, 487, 503, 535, and 566.
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -467,6 +467,125 @@ public void testAllManifestsTableHonorsIgnoreResiduals()
throws IOException {
validateTaskScanResiduals(scan2, true);
}
+ @TestTemplate
+ public void testAllManifestsTableNegatedPredicateOnNonSnapshotColumn()
throws IOException {
+ // 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()
throws IOException {
+ // 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() throws IOException {
+ // 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("content",
ManifestContent.DELETES.id()))));
+
+ /*
+ * NOT(reference_snapshot_id = firstSnapshotId OR content = DELETES)
+ *
+ * is rewritten as:
+ *
+ * reference_snapshot_id != firstSnapshotId
Review Comment:
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.
--
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]
