zeroshade commented on code in PR #2111:
URL: https://github.com/apache/iceberg-go/pull/2111#discussion_r4198575103
##########
table/transaction.go:
##########
@@ -2861,6 +2864,84 @@ func (t *Transaction) rewriteFilesWithFilter(ctx
context.Context, fs io.IO, upda
return nil
}
+// isNotTrueExpr returns an expression that holds exactly for the rows where
+// filter is not true, i.e. where it is false or NULL. Plain NOT(filter) is not
+// enough: under three-valued logic `v = 5` is NULL when v is NULL, and so is
+// NOT(v = 5), so a copy-on-write rewrite would drop rows that never matched.
+//
+// The result contains no NOT. Row-group pruning pushes a NOT down with
+// RewriteNotExpr, which turns NOT(x < 5) into x >= 5, and that is false for
+// NaN. Negating the predicates here keeps NaN rows in both places.
+func isNotTrueExpr(filter iceberg.BooleanExpression)
(iceberg.BooleanExpression, error) {
+ res, err := iceberg.VisitExpr(filter, isNotTrueVisitor{})
+ if err != nil {
+ return nil, err
+ }
+
+ return res.notTrue, nil
+}
+
+// truthExprs holds two never-NULL expressions for a sub-expression e:
+// notTrue holds iff e is false or NULL, notFalse holds iff e is true or NULL.
+type truthExprs struct {
+ notTrue, notFalse iceberg.BooleanExpression
+}
+
+type isNotTrueVisitor struct{}
+
+func (isNotTrueVisitor) VisitTrue() truthExprs {
+ return truthExprs{notTrue: iceberg.AlwaysFalse{}, notFalse:
iceberg.AlwaysTrue{}}
+}
+
+func (isNotTrueVisitor) VisitFalse() truthExprs {
+ return truthExprs{notTrue: iceberg.AlwaysTrue{}, notFalse:
iceberg.AlwaysFalse{}}
+}
+
+func (isNotTrueVisitor) VisitNot(child truthExprs) truthExprs {
+ return truthExprs{notTrue: child.notFalse, notFalse: child.notTrue}
+}
+
+func (isNotTrueVisitor) VisitAnd(left, right truthExprs) truthExprs {
+ return truthExprs{
+ notTrue: iceberg.NewOr(left.notTrue, right.notTrue),
+ notFalse: iceberg.NewAnd(left.notFalse, right.notFalse),
+ }
+}
+
+func (isNotTrueVisitor) VisitOr(left, right truthExprs) truthExprs {
+ return truthExprs{
+ notTrue: iceberg.NewAnd(left.notTrue, right.notTrue),
+ notFalse: iceberg.NewOr(left.notFalse, right.notFalse),
+ }
+}
+
+func (isNotTrueVisitor) VisitUnbound(pred iceberg.UnboundPredicate) truthExprs
{
+ negated := pred.Negate()
+ switch pred.Op() {
+ case iceberg.OpIsNull, iceberg.OpNotNull, iceberg.OpIsNan,
iceberg.OpNotNan:
+ // never evaluates to NULL
+ return truthExprs{notTrue: negated, notFalse: pred}
+ case iceberg.OpLT, iceberg.OpLTEQ, iceberg.OpGT, iceberg.OpGTEQ:
+ // NaN fails both x < 5 and x >= 5. IsNaN binds to AlwaysFalse
for
+ // non-floating-point terms.
+ negated = iceberg.NewOr(negated, iceberg.IsNaN(pred.Term()))
+ }
+
+ // Any other predicate is NULL when its term is NULL. Guard In and NotIn
+ // too: binding turns a set that ends up with one value into Equal or
+ // NotEqual, which are NULL for a NULL term.
Review Comment:
This guard also changes multi-valued `NotIn`, not just the single-value
collapse. `not(is_in(x, {1, 2}))` is true for a NULL `x`, so a scan with
`NotIn(x, 1, 2)` returns NULL rows and merge-on-read deletes them, while
copy-on-write now keeps them. The PR description says this goes to a separate
issue, but I don't see one filed yet. Please open it and link it from this
comment so the divergence is visible here.
##########
table/transaction.go:
##########
@@ -2861,6 +2864,84 @@ func (t *Transaction) rewriteFilesWithFilter(ctx
context.Context, fs io.IO, upda
return nil
}
+// isNotTrueExpr returns an expression that holds exactly for the rows where
+// filter is not true, i.e. where it is false or NULL. Plain NOT(filter) is not
+// enough: under three-valued logic `v = 5` is NULL when v is NULL, and so is
+// NOT(v = 5), so a copy-on-write rewrite would drop rows that never matched.
+//
+// The result contains no NOT. Row-group pruning pushes a NOT down with
+// RewriteNotExpr, which turns NOT(x < 5) into x >= 5, and that is false for
+// NaN. Negating the predicates here keeps NaN rows in both places.
+func isNotTrueExpr(filter iceberg.BooleanExpression)
(iceberg.BooleanExpression, error) {
+ res, err := iceberg.VisitExpr(filter, isNotTrueVisitor{})
+ if err != nil {
+ return nil, err
+ }
+
+ return res.notTrue, nil
+}
+
+// truthExprs holds two never-NULL expressions for a sub-expression e:
+// notTrue holds iff e is false or NULL, notFalse holds iff e is true or NULL.
+type truthExprs struct {
+ notTrue, notFalse iceberg.BooleanExpression
+}
+
+type isNotTrueVisitor struct{}
+
+func (isNotTrueVisitor) VisitTrue() truthExprs {
+ return truthExprs{notTrue: iceberg.AlwaysFalse{}, notFalse:
iceberg.AlwaysTrue{}}
+}
+
+func (isNotTrueVisitor) VisitFalse() truthExprs {
+ return truthExprs{notTrue: iceberg.AlwaysTrue{}, notFalse:
iceberg.AlwaysFalse{}}
+}
+
+func (isNotTrueVisitor) VisitNot(child truthExprs) truthExprs {
+ return truthExprs{notTrue: child.notFalse, notFalse: child.notTrue}
+}
+
+func (isNotTrueVisitor) VisitAnd(left, right truthExprs) truthExprs {
+ return truthExprs{
+ notTrue: iceberg.NewOr(left.notTrue, right.notTrue),
+ notFalse: iceberg.NewAnd(left.notFalse, right.notFalse),
+ }
+}
+
+func (isNotTrueVisitor) VisitOr(left, right truthExprs) truthExprs {
+ return truthExprs{
+ notTrue: iceberg.NewAnd(left.notTrue, right.notTrue),
+ notFalse: iceberg.NewOr(left.notFalse, right.notFalse),
+ }
+}
+
+func (isNotTrueVisitor) VisitUnbound(pred iceberg.UnboundPredicate) truthExprs
{
+ negated := pred.Negate()
+ switch pred.Op() {
+ case iceberg.OpIsNull, iceberg.OpNotNull, iceberg.OpIsNan,
iceberg.OpNotNan:
+ // never evaluates to NULL
+ return truthExprs{notTrue: negated, notFalse: pred}
Review Comment:
`IsNaN` is grouped with the never-NULL predicates, so `Delete(IsNaN(x))`
gets `NotNaN(x)` as its complement with no `IS NULL` guard. That holds while
`x` is in the file (`is_nan` on a NULL comes out false, so the row is kept). It
doesn't hold for data files written before `x` was added: for a missing column
with no initial default, `scanTranslator.VisitBound` maps every predicate
except `IsNull` to `AlwaysFalse`. The complement becomes `AlwaysFalse` and the
rewrite drops every row of those files. On v3 the same `AlwaysFalse` reaches
the row-group pruning filter, with the same result. With `NOT(filter)`,
`NOT(AlwaysFalse)` kept them.
Repro (copy-on-write, v2 and v3): append ids 1 and 2, add an optional `x
double`, then append `{3, 1.0}` and `{4, NaN}`. `Delete(IsNaN(x))` leaves `[3]`
on this branch and `[1 2 3]` on main. `NewAnd(IsNaN(x), GreaterThan(id, 0))`
behaves the same way.
`IsNaN` is false for NULL, so its not-true side should keep NULL rows the
same way the other predicates do:
```suggestion
case iceberg.OpIsNull, iceberg.OpNotNull, iceberg.OpNotNan:
// never evaluates to NULL
return truthExprs{notTrue: negated, notFalse: pred}
case iceberg.OpIsNan:
// is_nan(NULL) is false, so NULL rows survive. The guard also
covers
// files written before the column existed, where NotNaN on the
missing
// column translates to AlwaysFalse.
return truthExprs{notTrue: iceberg.NewOr(negated,
iceberg.IsNull(pred.Term())), notFalse: pred}
```
With that change applied locally, the repro keeps `[1 2 3]` and the four new
tests still pass. Please add the schema-evolution case to the tests.
--
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]
