Skip to content

Commit

Permalink
exec: fix NaN comparison logic
Browse files Browse the repository at this point in the history
I added special NaN handling for float comparisons. In SQL, NaNs are
treated as less than any other float value.

Thankfully I'm not seeing a performance hit when I run our sort
benchmarks with float64 values.

Fixes #38751

Release note: None
  • Loading branch information
solongordon committed Jul 15, 2019
1 parent d7232ed commit 540d2f8
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 2 deletions.
7 changes: 7 additions & 0 deletions pkg/sql/exec/execgen/cmd/execgen/overloads.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,13 @@ func (c floatCustomizer) getHashAssignFunc() assignFunc {
}
}

func (c floatCustomizer) getCmpOpCompareFunc() compareFunc {
// Float comparisons need special handling for NaN.
return func(l, r string) string {
return fmt.Sprintf("compareFloats(float64(%s), float64(%s))", l, r)
}
}

func (c intCustomizer) getHashAssignFunc() assignFunc {
return func(op overload, target, v, _ string) string {
return fmt.Sprintf("%[1]s = memhash%[3]d(noescape(unsafe.Pointer(&%[2]s)), %[1]s)", target, v, c.width)
Expand Down
35 changes: 35 additions & 0 deletions pkg/sql/exec/float.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright 2019 The Cockroach Authors.
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package exec

import "math"

// compareFloats compares two float values. This function is necessary for NaN
// handling. In SQL, NaN is treated as less than all other float values. In Go,
// any comparison with NaN returns false.
func compareFloats(a, b float64) int {
if a < b {
return -1
}
if a == b {
return 0
}
if a > b {
return 1
}
if math.IsNaN(a) {
if math.IsNaN(b) {
return 0
}
return -1
}
return 1
}
5 changes: 3 additions & 2 deletions pkg/sql/exec/sort_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ package exec
import (
"context"
"fmt"
"math"
"math/rand"
"sort"
"testing"
Expand Down Expand Up @@ -76,8 +77,8 @@ func TestSort(t *testing.T) {
ordCols: []distsqlpb.Ordering_Column{{ColIdx: 0}},
},
{
tuples: tuples{{3.2}, {2.0}, {2.4}},
expected: tuples{{2.0}, {2.4}, {3.2}},
tuples: tuples{{3.2}, {2.0}, {2.4}, {math.NaN()}, {math.Inf(-1)}, {math.Inf(1)}},
expected: tuples{{math.NaN()}, {math.Inf(-1)}, {2.0}, {2.4}, {3.2}, {math.Inf(1)}},
typ: []types.T{types.Float64},
ordCols: []distsqlpb.Ordering_Column{{ColIdx: 0}},
},
Expand Down
9 changes: 9 additions & 0 deletions pkg/sql/exec/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ package exec
import (
"context"
"fmt"
"math"
"math/rand"
"reflect"
"sort"
Expand Down Expand Up @@ -516,6 +517,14 @@ func tupleEquals(expected tuple, actual tuple) bool {
return false
}
} else {
// Special case for NaN, since it does not equal itself.
if f1, ok := expected[i].(float64); ok {
if f2, ok := actual[i].(float64); ok {
if math.IsNaN(f1) && math.IsNaN(f2) {
continue
}
}
}
if !reflect.DeepEqual(reflect.ValueOf(actual[i]).Convert(reflect.TypeOf(expected[i])).Interface(), expected[i]) {
return false
}
Expand Down

0 comments on commit 540d2f8

Please sign in to comment.