This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new 2c5bc2359 fix: keep NaN rows when RewriteNotExpr pushes a NOT down
(#2355)
2c5bc2359 is described below
commit 2c5bc23598aed1e160a8285f8af5c8c1bf33f507
Author: Revanth <[email protected]>
AuthorDate: Wed Nov 4 11:10:19 2026 -0600
fix: keep NaN rows when RewriteNotExpr pushes a NOT down (#2355)
* fix: keep NaN rows when RewriteNotExpr pushes a NOT down
* docs: link the follow-up issues and note the float comparison change
---
exprs.go | 40 ++++++
exprs_test.go | 27 ++--
literals.go | 9 +-
table/delete_classification_nan_test.go | 105 ++++++++++++++++
table/not_nan_pruning_test.go | 210 ++++++++++++++++++++++++++++++++
table/transaction_delete.go | 5 +-
visitors.go | 52 +++++++-
visitors_test.go | 59 +++++++++
website/src/releases.md | 1 +
9 files changed, 493 insertions(+), 15 deletions(-)
diff --git a/exprs.go b/exprs.go
index b133c9dd9..cb427a761 100644
--- a/exprs.go
+++ b/exprs.go
@@ -769,6 +769,30 @@ 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. Java still folds them, see
+ // https://github.com/apache/iceberg/issues/18700.
+ if isFloatingPoint(bound.Type()) && isOrderingOp(ul.op) {
+ switch lit.(type) {
+ case AboveMaxLiteral:
+ if ul.op == OpLT || ul.op == OpLTEQ {
+ return createBoundLiteralPredicate(OpLTEQ,
bound, maxFiniteLiteral(bound.Type()))
+ }
+
+ return createBoundLiteralPredicate(OpGT, bound,
maxFiniteLiteral(bound.Type()))
+ case BelowMinLiteral:
+ if ul.op == OpGT || ul.op == OpGTEQ {
+ return createBoundLiteralPredicate(OpGTEQ,
bound, minFiniteLiteral(bound.Type()))
+ }
+
+ return createBoundLiteralPredicate(OpLT, bound,
minFiniteLiteral(bound.Type()))
+ }
+ }
+
switch lit.(type) {
case AboveMaxLiteral:
switch ul.op {
@@ -1437,3 +1461,19 @@ func (b *boundBBoxPredicate) Equals(other
BooleanExpression) bool {
return b.op == rhs.op && b.term.Equals(rhs.term) &&
b.bbox.Equals(rhs.bbox)
}
+
+func maxFiniteLiteral(typ Type) Literal {
+ if _, ok := typ.(Float32Type); ok {
+ return Float32Literal(math.MaxFloat32)
+ }
+
+ return Float64Literal(math.MaxFloat64)
+}
+
+func minFiniteLiteral(typ Type) Literal {
+ if _, ok := typ.(Float32Type); ok {
+ return Float32Literal(-math.MaxFloat32)
+ }
+
+ return Float64Literal(-math.MaxFloat64)
+}
diff --git a/exprs_test.go b/exprs_test.go
index 5286b0f8a..dff135bb7 100644
--- a/exprs_test.go
+++ b/exprs_test.go
@@ -983,6 +983,12 @@ func TestBindAboveBelowIntMax(t *testing.T) {
ref, ref2 := iceberg.Reference("a"), iceberg.Reference("b")
above, below := int64(math.MaxInt32)+1, int64(math.MinInt32)-1
above2, below2 := float64(math.MaxFloat32)+1e37,
float64(-math.MaxFloat32)-1e37
+ boundFloat32 := func(pred iceberg.UnboundPredicate)
iceberg.BooleanExpression {
+ bound, err := pred.Bind(sc, true)
+ require.NoError(t, err)
+
+ return bound
+ }
tests := []struct {
pred iceberg.UnboundPredicate
@@ -1005,14 +1011,19 @@ func TestBindAboveBelowIntMax(t *testing.T) {
{iceberg.EqualTo(ref2, below2), iceberg.AlwaysFalse{}},
{iceberg.NotEqualTo(ref2, above2), iceberg.AlwaysTrue{}},
{iceberg.NotEqualTo(ref2, below2), iceberg.AlwaysTrue{}},
- {iceberg.LessThan(ref2, above2), iceberg.AlwaysTrue{}},
- {iceberg.LessThan(ref2, below2), iceberg.AlwaysFalse{}},
- {iceberg.LessThanEqual(ref2, above2), iceberg.AlwaysTrue{}},
- {iceberg.LessThanEqual(ref2, below2), iceberg.AlwaysFalse{}},
- {iceberg.GreaterThan(ref2, above2), iceberg.AlwaysFalse{}},
- {iceberg.GreaterThan(ref2, below2), iceberg.AlwaysTrue{}},
- {iceberg.GreaterThanEqual(ref2, above2), iceberg.AlwaysFalse{}},
- {iceberg.GreaterThanEqual(ref2, below2), iceberg.AlwaysTrue{}},
+ // a float comparison keeps NaN, the infinities and NULL, so it
moves
+ // to the largest finite value instead of folding
+ {iceberg.LessThan(ref2, above2),
boundFloat32(iceberg.LessThanEqual(ref2, float32(math.MaxFloat32)))},
+ {iceberg.LessThan(ref2, below2),
boundFloat32(iceberg.LessThan(ref2, float32(-math.MaxFloat32)))},
+ {iceberg.LessThanEqual(ref2, above2),
boundFloat32(iceberg.LessThanEqual(ref2, float32(math.MaxFloat32)))},
+ {iceberg.LessThanEqual(ref2, below2),
boundFloat32(iceberg.LessThan(ref2, float32(-math.MaxFloat32)))},
+ {iceberg.GreaterThan(ref2, above2),
boundFloat32(iceberg.GreaterThan(ref2, float32(math.MaxFloat32)))},
+ {iceberg.GreaterThan(ref2, below2),
boundFloat32(iceberg.GreaterThanEqual(ref2, float32(-math.MaxFloat32)))},
+ {iceberg.GreaterThanEqual(ref2, above2),
boundFloat32(iceberg.GreaterThan(ref2, float32(math.MaxFloat32)))},
+ {iceberg.GreaterThanEqual(ref2, below2),
boundFloat32(iceberg.GreaterThanEqual(ref2, float32(-math.MaxFloat32)))},
+ // infinity is a float32 value, so it doesn't fold
+ {iceberg.LessThanEqual(ref2, math.Inf(1)),
boundFloat32(iceberg.LessThanEqual(ref2, float32(math.Inf(1))))},
+ {iceberg.EqualTo(ref2, math.Inf(-1)),
boundFloat32(iceberg.EqualTo(ref2, float32(math.Inf(-1))))},
}
for _, tt := range tests {
diff --git a/literals.go b/literals.go
index c1f0b933b..1b65bdb81 100644
--- a/literals.go
+++ b/literals.go
@@ -680,9 +680,14 @@ func (f Float64Literal) String() string {
return strconv.FormatFl
func (f Float64Literal) To(t Type) (Literal, error) {
switch t := t.(type) {
case Float32Type:
- if math.MaxFloat32 < f {
+ switch {
+ case math.IsInf(float64(f), 0):
+ // infinity is a float32 value, unlike a finite value
beyond
+ // the float32 range, and x <= +Inf holds for +Inf
+ return Float32Literal(f), nil
+ case math.MaxFloat32 < f:
return Float32AboveMaxLiteral(), nil
- } else if -math.MaxFloat32 > f {
+ case -math.MaxFloat32 > f:
return Float32BelowMinLiteral(), nil
}
diff --git a/table/delete_classification_nan_test.go
b/table/delete_classification_nan_test.go
new file mode 100644
index 000000000..51292039e
--- /dev/null
+++ b/table/delete_classification_nan_test.go
@@ -0,0 +1,105 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "math"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// A delete drops a file whole when the strict evaluator says every row
+// matches. That must agree with a scan's row filter for files of only NaN,
+// only NULL or only +Inf, also when binding moves a literal beyond the float32
+// range. iceberg-go doesn't write NaN counts, but other engines write complete
+// ones.
+func TestDeleteWholeFileDecisionMatchesNaNRows(t *testing.T) {
+ schema := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "x", Type:
iceberg.PrimitiveTypes.Float32},
+ )
+ x := iceberg.Reference("x")
+ above, below := 1e100, -1e100
+
+ posInf, err :=
iceberg.Float32Literal(float32(math.Inf(1))).MarshalBinary()
+ require.NoError(t, err)
+
+ newFile := func(nulls, nans int64, bound []byte) iceberg.DataFile {
+ builder, err := iceberg.NewDataFileBuilder(
+ *iceberg.UnpartitionedSpec, iceberg.EntryContentData,
"file.parquet", iceberg.ParquetFile,
+ nil, nil, nil, 2, 100,
+ )
+ require.NoError(t, err)
+ builder.
+ ValueCounts(map[int]int64{1: 2}).
+ NullValueCounts(map[int]int64{1: nulls}).
+ NaNValueCounts(map[int]int64{1: nans})
+ if bound != nil {
+ builder.
+ LowerBoundValues(map[int][]byte{1: bound}).
+ UpperBoundValues(map[int][]byte{1: bound})
+ }
+
+ return builder.Build()
+ }
+ allNaN, allNull, allPosInf := newFile(0, 2, nil), newFile(2, 0, nil),
newFile(0, 0, posInf)
+
+ // whether a scan's row filter matches a NaN row and a +Inf row; it
never
+ // matches a NULL row, since every filter here is NULL for it
+ cases := []struct {
+ filter iceberg.BooleanExpression
+ nan, infinity bool
+ }{
+ {iceberg.LessThan(x, above), false, false},
+ {iceberg.NewNot(iceberg.LessThan(x, above)), true, true},
+ {iceberg.GreaterThan(x, above), false, true},
+ {iceberg.NewNot(iceberg.GreaterThan(x, above)), true, false},
+ {iceberg.GreaterThan(x, below), false, true},
+ {iceberg.NewNot(iceberg.GreaterThan(x, below)), true, false},
+ {iceberg.NewNot(iceberg.NewAnd(iceberg.LessThan(x, above),
iceberg.GreaterThan(x, below))), true, true},
+ {iceberg.NewNot(iceberg.NewOr(iceberg.GreaterThan(x, above),
iceberg.LessThan(x, below))), true, false},
+ {iceberg.LessThan(x, 5.0), false, false},
+ {iceberg.NewNot(iceberg.LessThan(x, 5.0)), true, true},
+ {iceberg.IsNaN(x), true, false},
+ }
+
+ for _, c := range cases {
+ t.Run(c.filter.String(), func(t *testing.T) {
+ notTrue, err := isNotTrueExpr(c.filter)
+ require.NoError(t, err)
+ strict, err := newStrictMetricsEvaluator(schema,
iceberg.NewNot(notTrue), true, false)
+ require.NoError(t, err)
+
+ for _, f := range []struct {
+ name string
+ file iceberg.DataFile
+ matches bool
+ }{
+ {"all NaN", allNaN, c.nan},
+ {"all NULL", allNull, false},
+ {"all +Inf", allPosInf, c.infinity},
+ } {
+ deleteWhole, err := strict(f.file)
+ require.NoError(t, err)
+ assert.Equal(t, f.matches, deleteWhole, f.name)
+ }
+ })
+ }
+}
diff --git a/table/not_nan_pruning_test.go b/table/not_nan_pruning_test.go
new file mode 100644
index 000000000..c8fb89eeb
--- /dev/null
+++ b/table/not_nan_pruning_test.go
@@ -0,0 +1,210 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table_test
+
+import (
+ "context"
+ "math"
+ "path/filepath"
+ "slices"
+ "testing"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/iceberg-go"
+ iceio "github.com/apache/iceberg-go/io"
+ "github.com/apache/iceberg-go/table"
+ "github.com/stretchr/testify/require"
+)
+
+// NOT(x < 5) is true for NaN, so the NaN row must match whatever the other
+// values in its file are. Min/max leave NaN out, and pruning on x >= 5 alone
+// skipped a file whose other values are all below 5. Scans and both delete
+// modes must agree.
+// https://github.com/apache/iceberg-go/issues/2131
+func TestNotComparisonMatchesNaNRegardlessOfFileBounds(t *testing.T) {
+ x := iceberg.Reference("x")
+ notLessThan5 := iceberg.NewNot(iceberg.LessThan(x, 5.0))
+
+ cases := []struct {
+ name string
+ isFloat32 bool
+ partitioned bool
+ values []float64
+ filter iceberg.BooleanExpression
+ want []int64
+ }{
+ {name: "bounds above", values: []float64{10, math.NaN(), 20},
filter: notLessThan5, want: []int64{1, 2, 3}},
+ {name: "bounds below", values: []float64{1, math.NaN(), 2},
filter: notLessThan5, want: []int64{2}},
+ {
+ name: "not and", values: []float64{1, math.NaN(), 2},
+ filter:
iceberg.NewNot(iceberg.NewAnd(iceberg.GreaterThan(x, 0.0), iceberg.LessThan(x,
5.0))),
+ want: []int64{2},
+ },
+ // each file is pruned by evaluating the projected filter on its
+ // partition value, NaN for the second row
+ {name: "identity partition", partitioned: true, values:
[]float64{1, math.NaN(), 2}, filter: notLessThan5, want: []int64{2}},
+ // binding folds a literal outside the float32 range, and NaN
still
+ // fails the comparison
+ {name: "float32 out of range", isFloat32: true, values:
[]float64{1, math.NaN(), 2}, filter: iceberg.NewNot(iceberg.LessThan(x,
1e100)), want: []int64{2}},
+ {name: "float32 out of range without not", isFloat32: true,
values: []float64{1, math.NaN(), 2}, filter: iceberg.LessThan(x, 1e100), want:
[]int64{1, 3}},
+ }
+
+ for _, mode := range []string{table.WriteModeCopyOnWrite,
table.WriteModeMergeOnRead} {
+ for _, c := range cases {
+ t.Run(mode+"/"+c.name, func(t *testing.T) {
+ tbl := newNaNTable(t, mode, c.isFloat32,
c.partitioned, c.values, nil)
+
+ requireScanAndDeleteMatch(t, tbl, c.filter,
c.want, len(c.values))
+ })
+ }
+ }
+}
+
+// An ordering comparison against a literal beyond the float32 range binds to
+// one against the largest finite float32, so NaN, the infinities and NULL
+// compare as they do against the literal itself, also under NOT.
+func TestFloat32OutOfRangeComparisonMatchesIEEE(t *testing.T) {
+ x := iceberg.Reference("x")
+ above, below, inf := 1e100, -1e100, math.Inf(1)
+
+ // ids 1 to 6
+ values := []float64{1, math.NaN(), inf, -inf, 0, 2}
+ valid := []bool{true, true, true, true, false, true}
+
+ cases := []struct {
+ filter iceberg.BooleanExpression
+ want []int64
+ }{
+ {iceberg.LessThan(x, above), []int64{1, 4, 6}},
+ {iceberg.NewNot(iceberg.LessThan(x, above)), []int64{2, 3}},
+ {iceberg.LessThanEqual(x, above), []int64{1, 4, 6}},
+ {iceberg.NewNot(iceberg.LessThanEqual(x, above)), []int64{2,
3}},
+ {iceberg.GreaterThan(x, above), []int64{3}},
+ {iceberg.NewNot(iceberg.GreaterThan(x, above)), []int64{1, 2,
4, 6}},
+ {iceberg.GreaterThanEqual(x, above), []int64{3}},
+ {iceberg.GreaterThan(x, below), []int64{1, 3, 6}},
+ {iceberg.NewNot(iceberg.GreaterThan(x, below)), []int64{2, 4}},
+ {iceberg.LessThan(x, below), []int64{4}},
+ {iceberg.NewNot(iceberg.LessThan(x, below)), []int64{1, 2, 3,
6}},
+ {iceberg.NewNot(iceberg.NewAnd(iceberg.LessThan(x, above),
iceberg.GreaterThan(x, below))), []int64{2, 3, 4}},
+ {iceberg.NewNot(iceberg.NewOr(iceberg.GreaterThan(x, above),
iceberg.LessThan(x, below))), []int64{1, 2, 6}},
+ {iceberg.LessThan(x, inf), []int64{1, 4, 6}},
+ {iceberg.LessThanEqual(x, inf), []int64{1, 3, 4, 6}},
+ {iceberg.GreaterThan(x, inf), []int64{}},
+ {iceberg.EqualTo(x, inf), []int64{3}},
+ {iceberg.NewNot(iceberg.LessThanEqual(x, inf)), []int64{2}},
+ }
+
+ for _, mode := range []string{table.WriteModeCopyOnWrite,
table.WriteModeMergeOnRead} {
+ for _, c := range cases {
+ t.Run(mode+"/"+c.filter.String(), func(t *testing.T) {
+ tbl := newNaNTable(t, mode, true, false,
values, valid)
+ requireScanAndDeleteMatch(t, tbl, c.filter,
c.want, len(values))
+ })
+ }
+ }
+}
+
+// requireScanAndDeleteMatch checks that a scan with filter returns want, and
+// that a delete with it removes exactly those rows.
+func requireScanAndDeleteMatch(t *testing.T, tbl *table.Table, filter
iceberg.BooleanExpression, want []int64, total int) {
+ t.Helper()
+ ctx := context.Background()
+
+ matched := scanIDsMatching(t, tbl, filter)
+ slices.Sort(matched)
+ require.Equal(t, want, matched)
+
+ txn := tbl.NewTransaction()
+ require.NoError(t, txn.Delete(ctx, filter, nil))
+ tbl, err := txn.Commit(ctx)
+ require.NoError(t, err)
+
+ remaining := scanIDs(t, tbl)
+ for _, id := range remaining {
+ require.NotContains(t, want, id)
+ }
+ require.Len(t, remaining, total-len(want))
+}
+
+// newNaNTable writes values to one data file per partition, with ids from 1. A
+// value is NULL where valid is false; a nil valid has no NULL.
+func newNaNTable(t *testing.T, mode string, isFloat32, partitioned bool,
values []float64, valid []bool) *table.Table {
+ t.Helper()
+
+ icebergType, arrowType := iceberg.Type(iceberg.PrimitiveTypes.Float64),
arrow.DataType(arrow.PrimitiveTypes.Float64)
+ if isFloat32 {
+ icebergType, arrowType = iceberg.PrimitiveTypes.Float32,
arrow.PrimitiveTypes.Float32
+ }
+ schema := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.PrimitiveTypes.Int64, Required: true},
+ iceberg.NestedField{ID: 2, Name: "x", Type: icebergType,
Required: false},
+ )
+ spec := iceberg.UnpartitionedSpec
+ if partitioned {
+ identity := iceberg.NewPartitionSpec(iceberg.PartitionField{
+ SourceIDs: []int{2}, FieldID: 1000, Name: "x",
Transform: iceberg.IdentityTransform{},
+ })
+ spec = &identity
+ }
+
+ location := filepath.ToSlash(t.TempDir())
+ meta, err := table.NewMetadata(schema, spec, table.UnsortedSortOrder,
location,
+ iceberg.Properties{
+ table.PropertyFormatVersion: "2",
+ table.WriteDeleteModeKey: mode,
+ })
+ require.NoError(t, err)
+
+ metaLoc := location + "/metadata/v1.metadata.json"
+ fsF := func(context.Context) (iceio.IO, error) { return
iceio.LocalFS{}, nil }
+ cat := &concurrentTestCatalog{metadata: meta, location: metaLoc, fsF:
fsF}
+ tbl := table.New(table.Identifier{"db", "nan_rows"}, meta, metaLoc,
fsF, cat)
+
+ bldr := array.NewRecordBuilder(memory.DefaultAllocator,
arrow.NewSchema([]arrow.Field{
+ {Name: "id", Type: arrow.PrimitiveTypes.Int64},
+ {Name: "x", Type: arrowType, Nullable: true},
+ }, nil))
+ defer bldr.Release()
+ for i, v := range values {
+ bldr.Field(0).(*array.Int64Builder).Append(int64(i + 1))
+ if valid != nil && !valid[i] {
+ bldr.Field(1).AppendNull()
+
+ continue
+ }
+ if isFloat32 {
+ bldr.Field(1).(*array.Float32Builder).Append(float32(v))
+ } else {
+ bldr.Field(1).(*array.Float64Builder).Append(v)
+ }
+ }
+ rec := bldr.NewRecordBatch()
+ defer rec.Release()
+
+ rdr, err := array.NewRecordReader(rec.Schema(),
[]arrow.RecordBatch{rec})
+ require.NoError(t, err)
+ defer rdr.Release()
+
+ tbl, err = tbl.Append(context.Background(), rdr, nil)
+ require.NoError(t, err)
+
+ return tbl
+}
diff --git a/table/transaction_delete.go b/table/transaction_delete.go
index aae3c0af1..f0a0cd053 100644
--- a/table/transaction_delete.go
+++ b/table/transaction_delete.go
@@ -628,9 +628,8 @@ func (t *Transaction) rewriteFilesWithFilter(ctx
context.Context, fs io.IO, upda
// 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.
+// The result contains no NOT. A negated ordering comparison gets an is_nan
+// branch, as RewriteNotExpr adds one, since NaN fails both x < 5 and x >= 5.
func isNotTrueExpr(filter iceberg.BooleanExpression)
(iceberg.BooleanExpression, error) {
res, err := iceberg.VisitExpr(filter, isNotTrueVisitor{})
if err != nil {
diff --git a/visitors.go b/visitors.go
index 6161c686c..ea4cd2943 100644
--- a/visitors.go
+++ b/visitors.go
@@ -564,7 +564,13 @@ 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;
+// see https://github.com/apache/iceberg-go/issues/2359.
func RewriteNotExpr(expr BooleanExpression) (BooleanExpression, error) {
return VisitExpr(expr, rewriteNotVisitor{})
}
@@ -574,7 +580,49 @@ type rewriteNotVisitor struct{}
func (rewriteNotVisitor) VisitTrue() BooleanExpression { return AlwaysTrue{} }
func (rewriteNotVisitor) VisitFalse() BooleanExpression { return AlwaysFalse{}
}
func (rewriteNotVisitor) VisitNot(child BooleanExpression) BooleanExpression {
- return child.Negate()
+ return negateKeepingNaN(child)
+}
+
+// negateKeepingNaN negates expr, which contains no Not nodes, adding an is_nan
+// branch to every negated ordering comparison on a floating-point term. And
+// and Or recurse here rather than through Negate, which would drop the branch
+// for their children.
+func negateKeepingNaN(expr BooleanExpression) BooleanExpression {
+ switch e := expr.(type) {
+ case AndExpr:
+ return NewOr(negateKeepingNaN(e.left),
negateKeepingNaN(e.right))
+ case OrExpr:
+ return NewAnd(negateKeepingNaN(e.left),
negateKeepingNaN(e.right))
+ case UnboundPredicate:
+ if isOrderingOp(e.Op()) {
+ // IsNaN binds to AlwaysFalse for a term that isn't
floating point.
+ return NewOr(e.Negate(), IsNaN(e.Term()))
+ }
+ case BoundPredicate:
+ if isOrderingOp(e.Op()) && isFloatingPoint(e.Term().Type()) {
+ return NewOr(e.Negate(),
createBoundUnaryPredicate(OpIsNan, e.Term()))
+ }
+ }
+
+ return expr.Negate()
+}
+
+func isOrderingOp(op Operation) bool {
+ switch op {
+ case OpLT, OpLTEQ, OpGT, OpGTEQ:
+ return true
+ }
+
+ return false
+}
+
+func isFloatingPoint(typ Type) bool {
+ switch typ.(type) {
+ case Float32Type, Float64Type:
+ return true
+ }
+
+ return false
}
func (rewriteNotVisitor) VisitAnd(left, right BooleanExpression)
BooleanExpression {
diff --git a/visitors_test.go b/visitors_test.go
index ce5cb82ba..884b3d3f0 100644
--- a/visitors_test.go
+++ b/visitors_test.go
@@ -1243,6 +1243,65 @@ func TestRewriteNot(t *testing.T) {
}
}
+// NaN fails both x < 5 and x >= 5, so a negated ordering comparison must keep
+// matching NaN: https://github.com/apache/iceberg-go/issues/2131
+func TestRewriteNotKeepsNaN(t *testing.T) {
+ schema := iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 1, Name: "x", Type:
iceberg.PrimitiveTypes.Float64},
+ iceberg.NestedField{ID: 2, Name: "f", Type:
iceberg.PrimitiveTypes.Float32},
+ iceberg.NestedField{ID: 3, Name: "i", Type:
iceberg.PrimitiveTypes.Int32},
+ )
+ x, f, i := iceberg.Reference("x"), iceberg.Reference("f"),
iceberg.Reference("i")
+
+ tests := []struct {
+ name string
+ expr, expected iceberg.BooleanExpression
+ }{
+ {"less than", iceberg.NewNot(iceberg.LessThan(x, 5.0)),
iceberg.NewOr(iceberg.GreaterThanEqual(x, 5.0), iceberg.IsNaN(x))},
+ {"less than or equal", iceberg.NewNot(iceberg.LessThanEqual(x,
5.0)), iceberg.NewOr(iceberg.GreaterThan(x, 5.0), iceberg.IsNaN(x))},
+ {"greater than", iceberg.NewNot(iceberg.GreaterThan(f,
float32(5))), iceberg.NewOr(iceberg.LessThanEqual(f, float32(5)),
iceberg.IsNaN(f))},
+ {"greater than or equal",
iceberg.NewNot(iceberg.GreaterThanEqual(x, 5.0)),
iceberg.NewOr(iceberg.LessThan(x, 5.0), iceberg.IsNaN(x))},
+ {"equal", iceberg.NewNot(iceberg.EqualTo(x, 5.0)),
iceberg.NotEqualTo(x, 5.0)},
+ {"is nan", iceberg.NewNot(iceberg.IsNaN(x)), iceberg.NotNaN(x)},
+ {"integer", iceberg.NewNot(iceberg.LessThan(i, int32(5))),
iceberg.GreaterThanEqual(i, int32(5))},
+ {
+ "not and",
+ iceberg.NewNot(iceberg.NewAnd(iceberg.GreaterThan(x,
0.0), iceberg.LessThan(i, int32(5)))),
+ iceberg.NewOr(iceberg.NewOr(iceberg.LessThanEqual(x,
0.0), iceberg.IsNaN(x)), iceberg.GreaterThanEqual(i, int32(5))),
+ },
+ {
+ "not or",
+ iceberg.NewNot(iceberg.NewOr(iceberg.LessThan(x, 0.0),
iceberg.GreaterThan(x, 5.0))),
+ iceberg.NewAnd(
+ iceberg.NewOr(iceberg.GreaterThanEqual(x, 0.0),
iceberg.IsNaN(x)),
+ iceberg.NewOr(iceberg.LessThanEqual(x, 5.0),
iceberg.IsNaN(x))),
+ },
+ }
+
+ for _, tt := range tests {
+ expected, err := iceberg.BindExpr(schema, tt.expected, true)
+ require.NoError(t, err)
+
+ t.Run("unbound/"+tt.name, func(t *testing.T) {
+ // the unbound rewrite has no type, so an integer term
gets an
+ // is_nan branch that binding then drops
+ out, err := iceberg.RewriteNotExpr(tt.expr)
+ require.NoError(t, err)
+ bound, err := iceberg.BindExpr(schema, out, true)
+ require.NoError(t, err)
+ assert.True(t, bound.Equals(expected), "got %s", bound)
+ })
+
+ t.Run("bound/"+tt.name, func(t *testing.T) {
+ bound, err := iceberg.BindExpr(schema, tt.expr, true)
+ require.NoError(t, err)
+ out, err := iceberg.RewriteNotExpr(bound)
+ require.NoError(t, err)
+ assert.True(t, out.Equals(expected), "got %s", out)
+ })
+ }
+}
+
func TestSanitizeExpression(t *testing.T) {
// A literal predicate must keep its column and operation but never
leak the
// literal a user scanned with.
diff --git a/website/src/releases.md b/website/src/releases.md
index 75490333d..92c9c6327 100644
--- a/website/src/releases.md
+++ b/website/src/releases.md
@@ -100,6 +100,7 @@ Per-release notes (highlights, breaking changes,
contributors) are published on
- **File writes and directory walks now take an operation context.** Update
`WriteFileIO` and `ListableIO` implementations and calls to `Create`,
`WriteFile`, and `WalkDir` to pass `context.Context`; cloud writes and listings
use it for cancellation.
- **In and NotIn follow SQL semantics for NULL.** A multi-valued `NotIn`,
`NOT(In(...))` and `NOT(NotIn(...))` no longer match rows where the column is
NULL, consistent with `NotEqual`. A predicate under NOT on a column missing
from an older data file no longer matches every row of that file; those rows
match as a stored NULL would. Java's `Evaluator` still matches NULL for `notEq`
and `notIn`, see
[apache/iceberg#18701](https://github.com/apache/iceberg/issues/18701).
- **Commit rejections now match `table.ErrCommitRejected`.** SQL, Glue, Hive
and Hadoop failures while preparing metadata, plus REST commit responses 400,
401, 403 and 404, leave transactions committable (after a REST 404, the next
`Commit` fails when it reloads the table). Existing `errors.Is` matches keep
working, but SQL, Glue, Hive and Hadoop rejection messages now start with
`commit rejected: `, so code that matches error text may need updating.
+- **Float comparisons against literals beyond the float32 range follow IEEE
754.** On a float32 column, `x < 1e100` no longer returns NaN, `+Inf` or NULL
rows, and `NOT(x > 1e100)` no longer returns NULL rows. An infinite literal now
binds to a float32 infinity, so `x = +Inf` matches `+Inf` rows. Java still
folds these comparisons to always true or false, see
[apache/iceberg#18700](https://github.com/apache/iceberg/issues/18700).
## Verifying and producing releases
