yangshangqing95 commented on PR #17346: URL: https://github.com/apache/iceberg/pull/17346#issuecomment-5092810732
> 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! -- 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]
