geo: coerce invalid geography coordinates into geometry

This commit resolves: cockroachdb#50219 Previously geometry did not convert out of range lat-lngs into correct range. This commit mimics the behavior found in PostGIS. It converts out of range lat-lngs into correct range. Release note (sql change): Geometry now coerce invalid geography coordinates into correct geometry.
sooryranga · Jul 1, 2020 · a22c16f · a22c16f
1 parent 7fa77eb
commit a22c16f
Show file tree

Hide file tree

Showing 4 changed files with 145 additions and 0 deletions.
diff --git a/pkg/geo/geo.go b/pkg/geo/geo.go
@@ -14,6 +14,7 @@ package geo
 import (
 	"bytes"
 	"encoding/binary"
+	"math"
 
 	"github.com/cockroachdb/cockroach/pkg/geo/geographiclib"
 	"github.com/cockroachdb/cockroach/pkg/geo/geopb"
@@ -617,6 +618,37 @@ func S2RegionsFromGeom(geomRepr geom.T, emptyBehavior EmptyBehavior) ([]s2.Regio
 // Common
 //
 
+// Normalize geographical coordinates
+// to spherical coordinates
+func makeValidGeographicalPoint(lat float64, lng float64) (float64, float64) {
+	if math.IsNaN(lat) || math.IsNaN(lng) {
+		return lat, lng
+	}
+	latlng := s2.LatLngFromDegrees(lat, lng)
+	return NormalizeLatitudeDegrees(latlng.Lat.Degrees()),
+		NormalizeLongitudeDegrees(latlng.Lng.Degrees())
+}
+
+// Limit geography coordinates to spherical coordinates
+// by converting geom.T cordinates inplace
+func makeValidGeographyGeom(t geom.T) {
+	if t.Layout() != geom.XY {
+		return
+	}
+
+	switch repr := t.(type) {
+	case *geom.GeometryCollection:
+		for _, geom := range repr.Geoms() {
+			makeValidGeographyGeom(geom)
+		}
+	default:
+		coords := repr.FlatCoords()
+		for i := 0; i < len(coords); i += repr.Stride() {
+			coords[i+1], coords[i] = makeValidGeographicalPoint(coords[i+1], coords[i])
+		}
+	}
+}
+
 // spatialObjectFromGeomT creates a geopb.SpatialObject from a geom.T.
 func spatialObjectFromGeomT(t geom.T, soType geopb.SpatialObjectType) (geopb.SpatialObject, error) {
 	ret, err := ewkb.Marshal(t, DefaultEWKBEncodingFormat)

diff --git a/pkg/geo/geo_test.go b/pkg/geo/geo_test.go
@@ -120,6 +120,78 @@ func TestGeospatialTypeFitsColumnMetadata(t *testing.T) {
 	}
 }
 
+func TestMakeValidGeographyGeom(t *testing.T) {
+	var (
+		invalidGeomPoint              = geom.NewPointFlat(geom.XY, []float64{200.0, 199.0})
+		invalidGeomLineString         = geom.NewLineStringFlat(geom.XY, []float64{90.0, 90.0, 180.0, 180.0})
+		invalidGeomPolygon            = geom.NewPolygonFlat(geom.XY, []float64{360.0, 360.0, 450.0, 450.0, 540.0, 540.0, 630.0, 630.0}, []int{8})
+		invalidGeomMultiPoint         = geom.NewMultiPointFlat(geom.XY, []float64{-90.0, -90.0, -180.0, -180.0})
+		invalidGeomMultiLineString    = geom.NewMultiLineStringFlat(geom.XY, []float64{-270.0, -270.0, -360.0, -360.0, -450.0, -450.0, -540.0, -540.0}, []int{4, 8})
+		invalidGeomMultiPolygon       = geom.NewMultiPolygon(geom.XY)
+		invalidGeomGeometryCollection = geom.NewGeometryCollection()
+	)
+	invalidGeomGeometryCollection.Push(geom.NewPointFlat(geom.XY, []float64{200.0, 199.0}))
+	invalidGeomGeometryCollection.Push(geom.NewLineStringFlat(geom.XY, []float64{90.0, 90.0, 180.0, 180.0}))
+	var (
+		validGeomPoint              = geom.NewPointFlat(geom.XY, []float64{-160.0, -19.0})
+		validGeomLineString         = geom.NewLineStringFlat(geom.XY, []float64{90.0, 90.0, 180.0, 0.0})
+		validGeomPolygon            = geom.NewPolygonFlat(geom.XY, []float64{0.0, 0.0, 90.0, 90.0, -180.0, 0.0, -90.0, -90.0}, []int{8})
+		validGeomMultiPoint         = geom.NewMultiPointFlat(geom.XY, []float64{-90.0, -90.0, -180.0, 0.0})
+		validGeomMultiLineString    = geom.NewMultiLineStringFlat(geom.XY, []float64{90.0, 90.0, 0.0, 0.0, -90.0, -90.0, 180.0, 0.0}, []int{4, 8})
+		validGeomMultiPolygon       = geom.NewMultiPolygon(geom.XY)
+		validGeomGeometryCollection = geom.NewGeometryCollection()
+	)
+	validGeomGeometryCollection.Push(validGeomPoint)
+	validGeomGeometryCollection.Push(validGeomLineString)
+	testCases := []struct {
+		desc string
+		g    geom.T
+		ret  geom.T
+	}{
+		{
+			"Point",
+			invalidGeomPoint,
+			validGeomPoint,
+		},
+		{
+			"linestring",
+			invalidGeomLineString,
+			validGeomLineString,
+		},
+		{
+			"polygon",
+			invalidGeomPolygon,
+			validGeomPolygon,
+		},
+		{
+			"multipoint",
+			invalidGeomMultiPoint,
+			validGeomMultiPoint,
+		},
+		{
+			"multilinestring",
+			invalidGeomMultiLineString,
+			validGeomMultiLineString,
+		},
+		{
+			"multipolygon",
+			invalidGeomMultiPolygon,
+			validGeomMultiPolygon,
+		},
+		{
+			"geometrycollection",
+			invalidGeomGeometryCollection,
+			validGeomGeometryCollection,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			makeValidGeographyGeom(tc.g)
+			require.Equal(t, tc.ret, tc.g)
+		})
+	}
+}
+
 func TestSpatialObjectFromGeomT(t *testing.T) {
 	testCases := []struct {
 		desc   string

diff --git a/pkg/geo/parse.go b/pkg/geo/parse.go
@@ -34,6 +34,9 @@ func parseEWKBRaw(soType geopb.SpatialObjectType, in geopb.EWKB) (geopb.SpatialO
 	if err != nil {
 		return geopb.SpatialObject{}, err
 	}
+	if soType == geopb.SpatialObjectType_GeographyType {
+		makeValidGeographyGeom(t)
+	}
 	return spatialObjectFromGeomT(t, soType)
 }
 
@@ -69,6 +72,9 @@ func parseEWKBHex(
 	if err != nil {
 		return geopb.SpatialObject{}, err
 	}
+	if soType == geopb.SpatialObjectType_GeographyType {
+		makeValidGeographyGeom(t)
+	}
 	if (defaultSRID != 0 && t.SRID() == 0) || int32(t.SRID()) < 0 {
 		adjustGeomSRID(t, defaultSRID)
 	}
@@ -87,6 +93,9 @@ func parseEWKB(
 	if err != nil {
 		return geopb.SpatialObject{}, err
 	}
+	if soType == geopb.SpatialObjectType_GeographyType {
+		makeValidGeographyGeom(t)
+	}
 	if overwrite == DefaultSRIDShouldOverwrite || (defaultSRID != 0 && t.SRID() == 0) || int32(t.SRID()) < 0 {
 		adjustGeomSRID(t, defaultSRID)
 	}
@@ -101,6 +110,9 @@ func parseWKB(
 	if err != nil {
 		return geopb.SpatialObject{}, err
 	}
+	if soType == geopb.SpatialObjectType_GeographyType {
+		makeValidGeographyGeom(t)
+	}
 	adjustGeomSRID(t, defaultSRID)
 	return spatialObjectFromGeomT(t, soType)
 }
@@ -113,6 +125,9 @@ func parseGeoJSON(
 	if err := geojson.Unmarshal(b, &t); err != nil {
 		return geopb.SpatialObject{}, err
 	}
+	if soType == geopb.SpatialObjectType_GeographyType {
+		makeValidGeographyGeom(t)
+	}
 	if defaultSRID != 0 && t.SRID() == 0 {
 		adjustGeomSRID(t, defaultSRID)
 	}

diff --git a/pkg/sql/logictest/testdata/logic_test/geospatial b/pkg/sql/logictest/testdata/logic_test/geospatial
@@ -144,6 +144,32 @@ SELECT ST_AsText(p) FROM (VALUES
 POINT (1 2)
 POINT (3 4)
 
+query T
+SELECT ST_AsText(p) FROM (VALUES
+  ('POINT(200 200)'::geography),
+  ('POLYGON((200 200, 200 200, 200 200, 200 200))'::geography),
+  ('GEOMETRYCOLLECTION(POINT (200 200), POLYGON((200 200, 200 200, 200 200, 200 200, 200 200)))'::geography),
+  ('MULTIPOLYGON(((200 200,200 200, 200 200, 200 200)),((200 200,200 200,200 200,200 200)))'::geography)
+) tbl(p)
+----
+POINT (-160 -20)
+POLYGON ((-160 -20, -160 -20, -160 -20, -160 -20))
+GEOMETRYCOLLECTION (POINT (-160 -20), POLYGON ((-160 -20, -160 -20, -160 -20, -160 -20, -160 -20)))
+MULTIPOLYGON (((-160 -20, -160 -20, -160 -20, -160 -20)), ((-160 -20, -160 -20, -160 -20, -160 -20)))
+
+query T
+SELECT ST_AsText(p) FROM (VALUES
+  ('POINT(200 200)'::geometry),
+  ('POLYGON((200 200, 200 200, 200 200, 200 200))'::geometry),
+  ('GEOMETRYCOLLECTION(POINT (200 200), POLYGON((200 200, 200 200, 200 200, 200 200, 200 200)))'::geometry),
+  ('MULTIPOLYGON(((200 200,200 200, 200 200, 200 200)),((200 200,200 200,200 200,200 200)))'::geometry)
+) tbl(p)
+----
+POINT (200 200)
+POLYGON ((200 200, 200 200, 200 200, 200 200))
+GEOMETRYCOLLECTION (POINT (200 200), POLYGON ((200 200, 200 200, 200 200, 200 200, 200 200)))
+MULTIPOLYGON (((200 200, 200 200, 200 200, 200 200)), ((200 200, 200 200, 200 200, 200 200)))
+
 query T
 SELECT ST_AsText(ST_Project('POINT(0 0)'::geography, 100000, radians(45.0)))
 ----