szehon-ho commented on code in PR #17346:
URL: https://github.com/apache/iceberg/pull/17346#discussion_r3649043848
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -467,6 +467,127 @@ public void testAllManifestsTableHonorsIgnoreResiduals()
throws IOException {
validateTaskScanResiduals(scan2, true);
}
+ @TestTemplate
+ public void testAllManifestsTableNegatedPredicateOnNonSnapshotColumn()
throws IOException {
+ List<Long> snapshotIds = prepareAllManifestsTableWithThreeSnapshots();
+
+ Table allManifestsTable = new AllManifestsTable(table);
+ TableScan scan =
+ allManifestsTable
+ .newScan()
+ .filter(Expressions.not(Expressions.equal("content",
ManifestContent.DATA.id())));
+
+ assertThat(plannedReferenceSnapshotIds(scan))
+ .as("A negated predicate on content must not prune snapshots")
+ .containsExactlyInAnyOrderElementsOf(snapshotIds);
+ }
+
+ @TestTemplate
+ public void testAllManifestsTableNotEqualPredicateOnNonSnapshotColumn()
throws IOException {
Review Comment:
These two cases already pass without the `AllManifestsTable` change. A
direct `notEqual` binds to a `NOT_EQ` predicate and goes through `notEq()`,
which returns `ROWS_MIGHT_MATCH` for non-snapshot refs, so the buggy `not()`
override is never reached. The same is true for
`testAllManifestsTableNegatedSnapshotOnlyPredicate` below, which also overlaps
with the existing `testAllManifestsTableSnapshotNot`.
I reverted `AllManifestsTable.java` to `main` and ran the suite: only
`testAllManifestsTableNegatedPredicateOnNonSnapshotColumn`,
`testAllManifestsTableNegatedOrPredicate`, and
`testAllManifestsTableNegatedAndPredicate` fail (12 failures = 3 tests x 4
format versions). Fine to keep the other two as guards, but worth knowing they
aren't regression coverage for this fix.
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -467,6 +467,127 @@ public void testAllManifestsTableHonorsIgnoreResiduals()
throws IOException {
validateTaskScanResiduals(scan2, true);
}
+ @TestTemplate
+ public void testAllManifestsTableNegatedPredicateOnNonSnapshotColumn()
throws IOException {
Review Comment:
Nit: new tests generally drop the `test` prefix, so
`allManifestsTableNegatedPredicateOnNonSnapshotColumn` and so on. The existing
methods here still carry it, so this is a local-consistency vs.
project-convention call — flagging it in case you'd rather follow the
convention for the new ones (and for
`testAllManifestsTableWithNegatedContentFilters` in the Spark tests).
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -1833,6 +1954,40 @@ public void testDeleteFilesTableScanOnBranch() throws
IOException {
.isEqualTo(2);
}
+ private List<Long> prepareAllManifestsTableWithThreeSnapshots() {
Review Comment:
Can these reuse what's already in the file? `preparePartitionedTableData()`
commits the same `FILE_A`/`FILE_C`/`FILE_D` sequence (plus `FILE_B`) with
deterministic snapshot ids 1-4, and `scannedPaths()` /
`expectedManifestListPaths()` in `MetadataTableScanTestBase` cover the
assertion side. Every neighboring `testAllManifestsTableSnapshot*` test is
written that way, e.g.:
```java
assertThat(scannedPaths(scan))
.isEqualTo(expectedManifestListPaths(table.snapshots(), 1L, 2L, 3L, 4L));
```
That would let both `prepareAllManifestsTableWithThreeSnapshots` and
`plannedReferenceSnapshotIds` go away and keep the assertions in one style
across the file.
--
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]
