Skip to content

Commit

Permalink
geo: fix nan handling in bounding box comparison
Browse files Browse the repository at this point in the history 
Go always returns false when comparing float NaNs, but SQL expects NaNs
to be less than any other float value. Before this change, the geo
package's `CartesianBoundingBox` did not have special handling for NaNs,
so it implemented the go behavior, which is incorrect for our use case.

This change adds correct NaN comparison behavior to bounding boxes.

Epic: None
Fixes: #93541
Fixes: #102661

Release note (bug fix): Fixes a bug in the geospatial cartesian bounding
box type that had incorrect behavior when comparing boxes with NaN
values.
  • Loading branch information
@rharding6373
rharding6373 committed Jun 28, 2023
1 parent 0997b24 commit b515d2d
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 10 deletions.
20 changes: 11 additions & 9 deletions pkg/geo/bbox.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/errors"
"github.com/golang/geo/s2"
"github.com/twpayne/go-geom"
geom "github.com/twpayne/go-geom"
)

// CartesianBoundingBox is the cartesian BoundingBox representation,
Expand Down Expand Up @@ -71,28 +71,30 @@ func ParseCartesianBoundingBox(s string) (CartesianBoundingBox, error) {

// Compare returns the comparison between two bounding boxes.
// Compare lower dimensions before higher ones, i.e. X, then Y.
// In SQL, NaN is treated as less than all other float values. In Go, any
// comparison with NaN returns false.
func (b *CartesianBoundingBox) Compare(o *CartesianBoundingBox) int {
if b.LoX < o.LoX {
if b.LoX < o.LoX || (math.IsNaN(b.LoX) && !math.IsNaN(o.LoX)) {
return -1
} else if b.LoX > o.LoX {
} else if b.LoX > o.LoX || (!math.IsNaN(b.LoX) && math.IsNaN(o.LoX)) {
return 1
}

if b.HiX < o.HiX {
if b.HiX < o.HiX || (math.IsNaN(b.HiX) && !math.IsNaN(o.HiX)) {
return -1
} else if b.HiX > o.HiX {
} else if b.HiX > o.HiX || (!math.IsNaN(b.HiX) && math.IsNaN(o.HiX)) {
return 1
}

if b.LoY < o.LoY {
if b.LoY < o.LoY || (math.IsNaN(b.LoY) && !math.IsNaN(o.LoY)) {
return -1
} else if b.LoY > o.LoY {
} else if b.LoY > o.LoY || (!math.IsNaN(b.LoY) && math.IsNaN(o.LoY)) {
return 1
}

if b.HiY < o.HiY {
if b.HiY < o.HiY || (math.IsNaN(b.HiY) && !math.IsNaN(o.HiY)) {
return -1
} else if b.HiY > o.HiY {
} else if b.HiY > o.HiY || (!math.IsNaN(b.HiY) && math.IsNaN(o.HiY)) {
return 1
}

Expand Down
114 changes: 113 additions & 1 deletion pkg/geo/bbox_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (

"github.com/cockroachdb/cockroach/pkg/geo/geopb"
"github.com/stretchr/testify/require"
"github.com/twpayne/go-geom"
geom "github.com/twpayne/go-geom"
)

func TestParseCartesianBoundingBox(t *testing.T) {
Expand Down Expand Up @@ -350,3 +350,115 @@ func TestCartesianBoundingBoxCovers(t *testing.T) {
})
}
}

func TestCartesianBoundingBoxCompare(t *testing.T) {
testCases := []struct {
desc string
a *CartesianBoundingBox
b *CartesianBoundingBox
expected int
}{
{
desc: "bounding box equals",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
expected: 0,
},
{
desc: "bounding box equals NaN",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: math.NaN(), HiX: math.NaN(), LoY: math.NaN(), HiY: math.NaN()}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: math.NaN(), HiX: math.NaN(), LoY: math.NaN(), HiY: math.NaN()}},
expected: 0,
},
{
desc: "left bounding box less",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: -1, HiX: 1, LoY: 0, HiY: 1}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
expected: -1,
},
{
desc: "right bounding box less",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 0.9, LoY: 1, HiY: 1}},
expected: 1,
},
{
desc: "left bounding box all NaN",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: math.NaN(), HiX: math.NaN(), LoY: math.NaN(), HiY: math.NaN()}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
expected: -1,
},
{
desc: "right bounding box all NaN",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: math.NaN(), HiX: math.NaN(), LoY: math.NaN(), HiY: math.NaN()}},
expected: 1,
},
{
desc: "left bounding box LoX NaN",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: math.NaN(), HiX: 1, LoY: 0, HiY: 1}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
expected: -1,
},
{
desc: "left bounding box HiX NaN",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: math.NaN(), LoY: 0, HiY: 1}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
expected: -1,
},
{
desc: "left bounding box LoY NaN",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: math.NaN(), HiY: 1}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
expected: -1,
},
{
desc: "left bounding box HiY NaN",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: math.NaN()}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
expected: -1,
},
{
desc: "right bounding box LoX NaN",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: math.NaN(), HiX: 1, LoY: 0, HiY: 1}},
expected: 1,
},
{
desc: "right bounding box HiX NaN",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: math.NaN(), LoY: 0, HiY: 1}},
expected: 1,
},
{
desc: "right bounding box LoY NaN",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: math.NaN(), HiY: 1}},
expected: 1,
},
{
desc: "right bounding box HiY NaN",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: math.NaN()}},
expected: 1,
},
{
desc: "left bounding box neg inf",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: math.Inf(-1), HiY: 1}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
expected: -1,
},
{
desc: "right bounding box pos inf",
a: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: 1}},
b: &CartesianBoundingBox{BoundingBox: geopb.BoundingBox{LoX: 0, HiX: 1, LoY: 0, HiY: math.Inf(1)}},
expected: -1,
},
}

for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
require.Equal(t, tc.expected, tc.a.Compare(tc.b))
})
}
}
16 changes: 16 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/geospatial_bbox
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,3 +293,19 @@ query T
SELECT ST_Box2DFromGeoHash(NULL, NULL)::BOX2D;
----
NULL

subtest regression_93541

statement ok
CREATE TABLE bbox_units (d INT PRIMARY KEY, f FLOAT4);

statement ok
INSERT INTO bbox_units VALUES (1, 1.0), (2, 'NaN');

query I
SELECT count(*) FROM bbox_units
WHERE 'BOX(-10 -10,10 10)'::BOX2D IN (
SELECT st_expand('BOX(1 -1, 1 -1)'::BOX2D, t2.f) FROM bbox_units t2
);
----
0

0 comments on commit b515d2d

Please sign in to comment.