Core: Fix pruning for negated all_manifests filters

[yangshangqing95]

Summary

Fixes #17345

Queries against the all_manifests metadata table can return incorrect results when a filter contains a negated predicate on a column that cannot be evaluated during snapshot planning.

For example, although delete manifests are present, the following queries will incorrectly return no rows:

SELECT *
FROM catalog.db.table.all_manifests
WHERE content != 0;

SELECT *
FROM catalog.db.table.all_manifests
WHERE NOT(content = 0);

AllManifestsTable.SnapshotEvaluator is a may-match evaluator. Predicates on columns other than reference_snapshot_id conservatively return true because matching manifest rows may exist. Negating that result directly changes true to false and incorrectly prunes the snapshot.

Changes

• Rewrite NOT expressions before binding filters in AllManifestsTable.SnapshotEvaluator.
• Preserve conservative evaluation for predicates that cannot be evaluated during snapshot planning.
• Preserve snapshot pruning for predicates on reference_snapshot_id.
• Add core regression tests for:
  • negated predicates on non-snapshot columns;
  • negated OR predicates containing known and unknown values;
  • negated AND predicates containing known and unknown values;
  • negated snapshot-only predicates.
• Add Spark 3.5, 4.0, 4.1 SQL regression coverage for:
  • content != 0;
  • content <> 0;
  • NOT(content = 0).

Why this fixes the issue

The evaluator previously handled an expression such as:

NOT(content = 0)

as:

content = 0        -> true because rows might match
NOT(content = 0)   -> false

The false result caused the snapshot to be incorrectly pruned.

Rewriting the expression first converts it to:

noteq

then leverage the noteq evaluator to do the filter.

The evaluator then conservatively keeps the snapshot because content cannot be determined while planning snapshot tasks.

Rewriting also preserves valid pruning for predicates that only reference reference_snapshot_id.

[uros-b]

LGTM, thank you @yangshangqing95! cc @szehon-ho who is familiar with this area and has reviewed similar fixes

[szehon-ho]

Thanks for the fix! I verified this is a real bug and that the approach is the right one.

I reproduced it in an isolated worktree by taking this PR's tests and reverting only AllManifestsTable.java to main: the three genuinely negated core tests fail across all four format versions, and the new Spark 4.1 test fails 6/6 — including plain WHERE content != 0, which returns zero rows on a table that has a delete manifest. Catalyst parses != as Not(EqualTo) and SparkV2Filters routes it through the NOT branch, so the user-facing impact described in the PR checks out. With the fix in place, TestMetadataTableScans, TestMetadataTableScansWithPartitionEvolution, TestEntriesMetadataTable, and the full Spark 4.1 TestMetadataTables all pass locally.

Rewriting NOT before binding is also consistent with the rest of the codebase — ManifestEvaluator, InclusiveMetricsEvaluator, StrictMetricsEvaluator, and BaseEntriesTable all call rewriteNot first, and AllManifestsTable was the only may-match evaluator that didn't. Dropping the not() override rather than leaving it dead is a nice touch. I also checked that rewriteNot is total over filter expressions (every predicate Operation reachable from a filter has a negation, and And/Or/Not/TRUE/FALSE implement negate() themselves), so there's no new failure path.

Only test-hygiene nits below, nothing blocking.

[szehon-ho]

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.

[yangshangqing95]

Yes the other two are guards

[szehon-ho]

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.:

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.

[yangshangqing95]

done

[szehon-ho]

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).

[yangshangqing95]

Thanks for flagging this. I’d prefer to keep the test prefix here for consistency with the existing methods in these files, so the new tests don’t stand out unnecessarily. If the project-wide naming convention is updated later, these can be renamed together as part of that cleanup.

[yangshangqing95]

Thanks for the fix! I verified this is a real bug and that the approach is the right one.

I reproduced it in an isolated worktree by taking this PR's tests and reverting only AllManifestsTable.java to main: the three genuinely negated core tests fail across all four format versions, and the new Spark 4.1 test fails 6/6 — including plain WHERE content != 0, which returns zero rows on a table that has a delete manifest. Catalyst parses != as Not(EqualTo) and SparkV2Filters routes it through the NOT branch, so the user-facing impact described in the PR checks out. With the fix in place, TestMetadataTableScans, TestMetadataTableScansWithPartitionEvolution, TestEntriesMetadataTable, and the full Spark 4.1 TestMetadataTables all pass locally.

Rewriting NOT before binding is also consistent with the rest of the codebase — ManifestEvaluator, InclusiveMetricsEvaluator, StrictMetricsEvaluator, and BaseEntriesTable all call rewriteNot first, and AllManifestsTable was the only may-match evaluator that didn't. Dropping the not() override rather than leaving it dead is a nice touch. I also checked that rewriteNot is total over filter expressions (every predicate Operation reachable from a filter has a negation, and And/Or/Not/TRUE/FALSE implement negate() themselves), so there's no new failure path.

Only test-hygiene nits below, nothing blocking.

Hi @uros-b @szehon-ho , thanks for the reviewing!

[szehon-ho]

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.

[yangshangqing95]

Yes, this exception was caused by the legacy helper method. It should have been removed together with the helper method, but it was overlooked. Fixed.

[szehon-ho]

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]

done