Skip to content

Commit

Permalink
Merge pull request #105829 from cockroachdb/blathers/backport-release…
Browse files Browse the repository at this point in the history 
…-22.2-105789

release-22.2: geo: fix nan handling in bounding box comparison
  • Loading branch information
@rharding6373
rharding6373 authored Jun 29, 2023
2 parents 552ade7 + 2e4bd1d commit 2decc53
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 2decc53

Please sign in to comment.