zeroshade commented on code in PR #2356:
URL: https://github.com/apache/iceberg-go/pull/2356#discussion_r4415239081
##########
visitors.go:
##########
@@ -679,7 +680,45 @@ func TranslateColumnNamesForScan(expr BooleanExpression,
fileSchema *Schema) (Bo
return nil, nil, err
}
- return res, tr.columns, nil
+ return resolveNullColumns(res, false), tr.columns, nil
+}
+
+// nullColumn stands for a predicate over a column that is null for every row
+// of a file, which is NULL rather than false. resolveNullColumns replaces it
+// before translation returns.
+type nullColumn struct{}
+
+func (nullColumn) String() string { return "NullColumn()" }
+func (nullColumn) Op() Operation { return OpFalse }
Review Comment:
Non-blocking: `Op()` reports `OpFalse` for a value that means NULL. It's
resolved before translation returns, so this is safe today, but future code
that checks `Op()` during translation would treat it as false. A short comment
saying it must not escape `TranslateColumnNamesForScan` would help.
##########
table/transaction_delete.go:
##########
@@ -691,11 +691,8 @@ func (isNotTrueVisitor) VisitUnbound(pred
iceberg.UnboundPredicate) truthExprs {
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. A scan with a multi-valued
- // NotIn still matches NULL rows, see
- // https://github.com/apache/iceberg-go/issues/2132.
+ // Any other predicate is NULL when its term is NULL, In and NotIn
Review Comment:
Non-blocking: please add a line under "Unreleased compatibility notes" in
`website/src/releases.md`. A multi-valued `NotIn`, and `NOT(In(...))`, no
longer match NULL rows, and a predicate under NOT on a column missing from a
file no longer matches every row of that file.
##########
table/substrait/substrait.go:
##########
@@ -405,12 +405,23 @@ func (t *toSubstraitExpr) makeSetFunc(id extensions.ID,
term iceberg.BoundTerm,
t.bldr.Literal(expr.NewNestedLiteral(val, false)))
}
+// VisitIn is NULL for a NULL term, as SQL IN and Equal are. Arrow's is_in is
+// false for it, so it is or-ed with (is_null(x) AND NULL), which is false for
+// any other value. NotIn negates the result, so it never matches NULL either.
func (t *toSubstraitExpr) VisitIn(term iceberg.BoundTerm, lits
iceberg.Set[iceberg.Literal]) expr.Builder {
Review Comment:
Diverging from Java's row `Evaluator` here is fine: Java returns true for
`notIn(null)`, and this follows SQL's three-valued logic instead. 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]
