Revanth14 commented on code in PR #2355: URL: https://github.com/apache/iceberg-go/pull/2355#discussion_r4415673970
########## visitors.go: ########## @@ -564,7 +564,12 @@ var _ BoundGeospatialExprVisitor[bool] = (*exprEvaluator)(nil) // RewriteNotExpr rewrites a boolean expression to remove "Not" nodes from the expression // tree. This is because Projections assume there are no "not" nodes. // -// Not nodes will be replaced with simply calling `Negate` on the child in the tree. +// Not nodes are replaced by negating the child in the tree. A negated ordering +// comparison also matches NaN, so NOT(x < 5) becomes x >= 5 OR is_nan(x): NaN +// fails both x < 5 and x >= 5, and the result must hold for the same rows as +// the NOT it replaces. That follows IEEE 754 comparison, as a scan's row +// filter does. ExpressionEvaluator orders NaN below every number instead, so on +// a NaN value it can disagree with the expression it evaluates before rewriting. Review Comment: Filed #2359 for the `ExpressionEvaluator` NaN ordering and linked it in the `RewriteNotExpr` doc. I checked that scans aren't affected today, since partition residuals keep the row filter for NaN. Filed #2360 for writing NaN value counts, which would bring back min/max pruning for `NOT(<float comparison>)` on files iceberg-go wrote. -- 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]
