eneskeles commented on PR #2111: URL: https://github.com/apache/iceberg-go/pull/2111#issuecomment-6044178165
> Thanks for turning this around. The strict evaluator rebuilt as `NewNot(isNotTrueExpr(filter))`, the NULL-matching note on both `Delete` and `Overwrite`, and the out-of-bounds cases are all in, and they cover what I was worried about last round. The visitor itself reads correctly: the De Morgan duals in `VisitAnd`/`VisitOr` and the NaN/NULL leaf guards all check out. > > Two test gaps are what's left for me, both cheap. The strict-eval rebuild is the riskiest part of this change and nothing asserts which path it took: the outside-bounds cases only check surviving ids, so a misclassification that still rewrote the right rows would pass. A snapshot-summary assertion (`deleted-data-files` / `added-data-files`), or a two-file table where one file drops whole and one is rewritten, would pin it. The other is the missing-column path: `NotNaN` / `NOT(IsNaN)` go through the no-guard branch, and that assumption isn't exercised in `TestCopyOnWriteDeleteKeepsRowsOfFilesWithoutTheColumn`, which is the one spot where a wrong guard is silent row loss on a schema-evolved table. > > The #2132 `NotIn` inconsistency and the all-NULL no-op are fine to leave tracked. One ask for the PR description: the MoR classification is now stricter (`NotEqual`/`NotIn` no longer drop files whole when they have NULLs or lack nan stats) and NaN ordering follows Arrow rather than Java, both defensible, but worth a changelog line so callers who used `Delete(NotEqualTo(...))` to purge NULLs know it changed. > > Add those two tests and I'm happy to approve. Thank you for review! I added those two tests and addressed the other catches. I also added a section to the PR description about the merge on read becoming stricter too - the "Behaviour changes for callers" section. The tests show one thing worth flagging: NotNaN(x) and NOT(IsNaN(x)) delete the rows of the old file rather than keeping them, so the cases expect only the NaN row to survive. NotNaN is true for a NULL in the evaluator, same as Java, so those rows match the filter and the no-guard branch is correct. Extending the IsNull guard to NotNaN would also keep NULL rows on files that have the column, which breaks the existing scan-matching case with a real NULL. -- 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]
