zeroshade commented on code in PR #2355:
URL: https://github.com/apache/iceberg-go/pull/2355#discussion_r4415211380
##########
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:
Non-blocking: please link the issue for aligning `ExpressionEvaluator`'s NaN
ordering. Separately, iceberg-go never fills in NaN value counts, so an
inclusive `is_nan` can't rule out a file iceberg-go wrote. That means
`NOT(<float comparison>)` loses min/max pruning on those files. Writing NaN
counts, as Java does, would bring it back and deserves a follow-up issue.
##########
exprs.go:
##########
@@ -769,6 +769,29 @@ func (ul *unboundLiteralPredicate) Bind(schema *Schema,
caseSensitive bool) (Boo
return nil, err
}
+ // A finite literal beyond the range of a floating-point term lies
between
+ // the largest finite value and infinity, so an ordering comparison
against
+ // it is the same as one against the largest finite value. Folding it to
+ // AlwaysTrue or AlwaysFalse instead would get NaN, the infinities and
NULL
+ // wrong: x < 1e100 on a float32 column is false for NaN and +Inf and
NULL
+ // for NULL.
+ if isFloatingPoint(bound.Type()) && isOrderingOp(ul.op) {
Review Comment:
Diverging from Java here is fine. Java folds these comparisons to
`alwaysTrue`/`alwaysFalse`, which gets NaN, `+Inf` and NULL wrong, as the
comment above says. Please file an issue on apache/iceberg asking for the
equivalent change so the implementations agree again, and mention it in this
comment and in the `releases.md` note.
--
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]
