Revanth14 opened a new pull request, #2355: URL: https://github.com/apache/iceberg-go/pull/2355
Closes #2131 ## Problem `RewriteNotExpr` negates predicates directly, so `NOT(x < 5)` becomes `x >= 5`. Both are false for NaN under IEEE, but the row filter evaluates `NOT(x < 5)` as written and matches NaN. Min/max stats and partition values let pruning skip files whose NaN rows the row filter would match, so scans and both delete modes depended on the other values in the file. ## Fix `RewriteNotExpr` adds an `is_nan` branch to every negated ordering comparison, so `NOT(x < 5)` becomes `x >= 5 OR is_nan(x)`, and nested And/Or keep the branch. For unbound predicates `IsNaN` binds to `AlwaysFalse` on non-floating-point terms, so this lives in `RewriteNotExpr` itself rather than after binding. Pruning rewrites before binding while a scan binds the filter as written, so binding has to agree, and it didn't for literals beyond the float32 range. Binding folded `x < 1e100` on a float32 column to `AlwaysTrue`, which is wrong for NaN, `+Inf` and NULL, and made the two orders disagree. An ordering comparison against such a literal now binds to one against the largest finite value: `x < 1e100` becomes `x <= MaxFloat32` and `x > 1e100` becomes `x > MaxFloat32`. An infinite literal now converts to float32 `±Inf` exactly instead of to `AboveMax`/`BelowMin`, so `x <= +Inf` keeps `+Inf` rows. **Behavior change:** on a float32 column, `x < 1e100` no longer returns NaN, `+Inf` or NULL rows, and `NOT(x > 1e100)` no longer returns NULL rows. Java folds these to `alwaysTrue`/`alwaysFalse`, so this diverges from it deliberately. Trade-offs: Parquet row-group stats carry no NaN counts, so `NOT(x < 5)` on a float column no longer prunes row groups on min/max alone. The strict metrics evaluator can't prove an `OR is_nan` matches every row of a file that mixes NaN and other values, so a delete may rewrite a file it used to drop whole. `ExpressionEvaluator` orders NaN below every number, unlike the row filter. Its in-tree use on rewritten expressions is partition pruning, which this fixes (an identity partition of NaN was pruned for `NOT(x < 5)`). Aligning it is left to a separate issue. ## Testing - `TestRewriteNotKeepsNaN`: rewritten shape for unbound and bound predicates, nested And/Or, and controls. - `TestFloat32OutOfRangeComparisonMatchesIEEE`: a file of 1, NaN, ±Inf, NULL and 2, with every out-of-range ordering comparison, negated and nested, plus infinite literals; scan, then delete under copy-on-write and merge-on-read. - `TestNotComparisonMatchesNaNRegardlessOfFileBounds`: the issue's repro and an identity partition, under both delete modes. - `TestDeleteWholeFileDecisionMatchesNaNRows`: the strict whole-file decision on files of only NaN, only NULL and only `+Inf`, with complete counts. - `TestBindAboveBelowIntMax`: updated for the new binding. ## Context Java's row `Evaluator` doesn't rewrite NOT and orders NaN above every number, so it has the same split between rows and pruning and isn't a reference here. -- 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]
