geo/geo*fn: handle EMPTY cases in GEOMETRY/GEOGRAPHY

There are many cases of EMPTY that we need to handle for each method we
support. This includes `POINT EMPTY`, `LINESTRING EMPTY` ...
`GEOMETRYCOLLECTION EMPTY` (i.e. one for each shape). Furthermore,
there's an even more confusing GEOMETRYCOLLECTION with EMPTY shapes,
e.g. `GEOMETRYCOLLECTION ( LINESTRING EMPTY ) `

Both GEOS and PostGIS have various bugs in handling these, which have
been commented in code and imitated where appropriate.

Also bump twpayne/go-geom to pickup the fixed Empty() checks.

Release note: None

pkg/geo/errors.go

@@ -10,7 +10,11 @@
 
 package geo
 
-import "github.com/cockroachdb/errors"
+import (
+	"fmt"
+
+	"github.com/cockroachdb/errors"
+)
 
 // NewMismatchingSRIDsError returns the error message for SRIDs of GeospatialTypes
 // a and b being a mismatch.
@@ -23,3 +27,42 @@ func NewMismatchingSRIDsError(a GeospatialType, b GeospatialType) error {
 		b.SRID(),
 	)
 }
+
+// EmptyGeometryError is an error that is returned when the Geometry or any
+// parts of its subgeometries are empty.
+type EmptyGeometryError struct {
+	cause error
+}
+
+var _ error = (*EmptyGeometryError)(nil)
+var _ errors.SafeDetailer = (*EmptyGeometryError)(nil)
+var _ fmt.Formatter = (*EmptyGeometryError)(nil)
+var _ errors.Formatter = (*EmptyGeometryError)(nil)
+
+// Error implements the error interface.
+func (w *EmptyGeometryError) Error() string { return w.cause.Error() }
+
+// Cause implements the errors.SafeDetailer interface.
+func (w *EmptyGeometryError) Cause() error { return w.cause }
+
+// Unwrap implements the SafeDetailer interface.
+func (w *EmptyGeometryError) Unwrap() error { return w.cause }
+
+// SafeDetails implements the SafeDetailer interface.
+func (w *EmptyGeometryError) SafeDetails() []string { return []string{w.cause.Error()} }
+
+// Format implements the errors.Formatter interface.
+func (w *EmptyGeometryError) Format(s fmt.State, verb rune) { errors.FormatError(w, s, verb) }
+
+// FormatError implements the errors.Formatter interface.
+func (w *EmptyGeometryError) FormatError(p errors.Printer) (next error) { return w.cause }
+
+// IsEmptyGeometryError returns true if the error is of type EmptyGeometryError.
+func IsEmptyGeometryError(err error) bool {
+	return errors.HasType(err, &EmptyGeometryError{})
+}
+
+// NewEmptyGeometryError returns an error indicating an empty geometry has been found.
+func NewEmptyGeometryError() *EmptyGeometryError {
+	return &EmptyGeometryError{cause: errors.Newf("empty shape found")}
+}

pkg/geo/geo.go

@@ -24,6 +24,17 @@ import (
 // DefaultEWKBEncodingFormat is the default encoding format for EWKB.
 var DefaultEWKBEncodingFormat = binary.LittleEndian
 
+// EmptyBehavior is the behavior to adopt when an empty Geometry is encountered.
+type EmptyBehavior uint8
+
+const (
+	// EmptyBehaviorError will error with EmptyGeometryError when an empty geometry
+	// is encountered.
+	EmptyBehaviorError EmptyBehavior = 0
+	// EmptyBehaviorOmit will omit an entry when an empty geometry is encountered.
+	EmptyBehaviorOmit EmptyBehavior = 1
+)
+
 //
 // Geospatial Type
 //
@@ -353,14 +364,14 @@ func (g *Geography) Shape() geopb.Shape {
 	return g.SpatialObject.Shape
 }
 
-// AsS2 converts a given Geography into its S2 form.
-func (g *Geography) AsS2() ([]s2.Region, error) {
+// AsS2 converts a given Geography into it's S2 form.
+func (g *Geography) AsS2(emptyBehavior EmptyBehavior) ([]s2.Region, error) {
 	geomRepr, err := g.AsGeomT()
 	if err != nil {
 		return nil, err
 	}
 	// TODO(otan): convert by reading from EWKB to S2 directly.
-	return S2RegionsFromGeom(geomRepr), nil
+	return S2RegionsFromGeom(geomRepr, emptyBehavior)
 }
 
 // isLinearRingCCW returns whether a given linear ring is counter clock wise.
@@ -407,8 +418,20 @@ func isLinearRingCCW(linearRing *geom.LinearRing) bool {
 
 // S2RegionsFromGeom converts an geom representation of an object
 // to s2 regions.
-func S2RegionsFromGeom(geomRepr geom.T) []s2.Region {
+// As S2 does not really handle empty geometries well, we need to ingest emptyBehavior and
+// react appropriately.
+func S2RegionsFromGeom(geomRepr geom.T, emptyBehavior EmptyBehavior) ([]s2.Region, error) {
 	var regions []s2.Region
+	if geomRepr.Empty() {
+		switch emptyBehavior {
+		case EmptyBehaviorOmit:
+			return nil, nil
+		case EmptyBehaviorError:
+			return nil, NewEmptyGeometryError()
+		default:
+			return nil, errors.Newf("programmer error: unknown behavior")
+		}
+	}
 	switch repr := geomRepr.(type) {
 	case *geom.Point:
 		regions = []s2.Region{
@@ -446,22 +469,38 @@ func S2RegionsFromGeom(geomRepr geom.T) []s2.Region {
 		}
 	case *geom.GeometryCollection:
 		for _, geom := range repr.Geoms() {
-			regions = append(regions, S2RegionsFromGeom(geom)...)
+			subRegions, err := S2RegionsFromGeom(geom, emptyBehavior)
+			if err != nil {
+				return nil, err
+			}
+			regions = append(regions, subRegions...)
 		}
 	case *geom.MultiPoint:
 		for i := 0; i < repr.NumPoints(); i++ {
-			regions = append(regions, S2RegionsFromGeom(repr.Point(i))...)
+			subRegions, err := S2RegionsFromGeom(repr.Point(i), emptyBehavior)
+			if err != nil {
+				return nil, err
+			}
+			regions = append(regions, subRegions...)
 		}
 	case *geom.MultiLineString:
 		for i := 0; i < repr.NumLineStrings(); i++ {
-			regions = append(regions, S2RegionsFromGeom(repr.LineString(i))...)
+			subRegions, err := S2RegionsFromGeom(repr.LineString(i), emptyBehavior)
+			if err != nil {
+				return nil, err
+			}
+			regions = append(regions, subRegions...)
 		}
 	case *geom.MultiPolygon:
 		for i := 0; i < repr.NumPolygons(); i++ {
-			regions = append(regions, S2RegionsFromGeom(repr.Polygon(i))...)
+			subRegions, err := S2RegionsFromGeom(repr.Polygon(i), emptyBehavior)
+			if err != nil {
+				return nil, err
+			}
+			regions = append(regions, subRegions...)
 		}
 	}
-	return regions
+	return regions, nil
 }
 
 //

pkg/geo/geo_test.go

@@ -421,12 +421,58 @@ func TestGeographyAsS2(t *testing.T) {
 			g, err := ParseGeography(tc.wkt)
 			require.NoError(t, err)
 
-			shapes, err := g.AsS2()
+			shapes, err := g.AsS2(EmptyBehaviorError)
 			require.NoError(t, err)
 
 			require.Equal(t, tc.expected, shapes)
 		})
 	}
+
+	// Test when things are empty.
+	emptyTestCases := []struct {
+		wkt          string
+		expectedOmit []s2.Region
+	}{
+		{
+			"GEOMETRYCOLLECTION ( LINESTRING EMPTY, MULTIPOINT((1.0 5.0), (3.0 4.0)) )",
+			[]s2.Region{
+				s2.PointFromLatLng(s2.LatLngFromDegrees(5.0, 1.0)),
+				s2.PointFromLatLng(s2.LatLngFromDegrees(4.0, 3.0)),
+			},
+		},
+		{
+			"GEOMETRYCOLLECTION EMPTY",
+			nil,
+		},
+		{
+			"MULTILINESTRING (EMPTY, (1.0 2.0, 3.0 4.0))",
+			[]s2.Region{
+				s2.PolylineFromLatLngs([]s2.LatLng{
+					s2.LatLngFromDegrees(2.0, 1.0),
+					s2.LatLngFromDegrees(4.0, 3.0),
+				}),
+			},
+		},
+		{
+			"MULTILINESTRING (EMPTY, EMPTY)",
+			nil,
+		},
+	}
+
+	for _, tc := range emptyTestCases {
+		t.Run(tc.wkt, func(t *testing.T) {
+			g, err := ParseGeography(tc.wkt)
+			require.NoError(t, err)
+
+			_, err = g.AsS2(EmptyBehaviorError)
+			require.Error(t, err)
+			require.True(t, IsEmptyGeometryError(err))
+
+			shapes, err := g.AsS2(EmptyBehaviorOmit)
+			require.NoError(t, err)
+			require.Equal(t, tc.expectedOmit, shapes)
+		})
+	}
 }
 
 func TestClipEWKBByRect(t *testing.T) {

pkg/geo/geogfn/covers.go

@@ -48,12 +48,17 @@ func Covers(a *geo.Geography, b *geo.Geography) (bool, error) {
 
 // covers is the internal calculation for Covers.
 func covers(a *geo.Geography, b *geo.Geography) (bool, error) {
-	aRegions, err := a.AsS2()
+	// Ignore EMPTY regions in a.
+	aRegions, err := a.AsS2(geo.EmptyBehaviorOmit)
 	if err != nil {
 		return false, err
 	}
-	bRegions, err := b.AsS2()
+	// If any of b is empty, we cannot cover it. Error and catch to return false.
+	bRegions, err := b.AsS2(geo.EmptyBehaviorError)
 	if err != nil {
+		if geo.IsEmptyGeometryError(err) {
+			return false, nil
+		}
 		return false, err
 	}
 

pkg/geo/geogfn/covers_test.go

@@ -354,6 +354,30 @@ func TestCovers(t *testing.T) {
 			"MULTIPOINT((0.5 0.5), (1.5 0.5))'",
 			true,
 		},
+		{
+			"EMPTY GEOMETRYCOLLECTION does not cover itself",
+			"GEOMETRYCOLLECTION EMPTY",
+			"GEOMETRYCOLLECTION EMPTY",
+			false,
+		},
+		{
+			"nothing covers an empty GEOMETRYCOLLECTION",
+			"POINT(1.0 1.0)",
+			"GEOMETRYCOLLECTION EMPTY",
+			false,
+		},
+		{
+			"nothing covers a GEOMETRYCOLLECTION with an EMPTY element",
+			"POINT(1.0 1.0)",
+			"GEOMETRYCOLLECTION EMPTY",
+			false,
+		},
+		{
+			"empty collection contains point which covers another",
+			"GEOMETRYCOLLECTION(LINESTRING EMPTY, POINT(1.0 2.0))",
+			"POINT(1.0 2.0)",
+			true,
+		},
 	}
 
 	for _, tc := range testCases {

pkg/geo/geogfn/distance.go

@@ -22,18 +22,19 @@ import (
 )
 
 // Distance returns the distance between geographies a and b on a sphere or spheroid.
+// Returns a geo.EmptyGeometryError if any of the Geographies are EMPTY.
 func Distance(
 	a *geo.Geography, b *geo.Geography, useSphereOrSpheroid UseSphereOrSpheroid,
 ) (float64, error) {
 	if a.SRID() != b.SRID() {
 		return 0, geo.NewMismatchingSRIDsError(a, b)
 	}
 
-	aRegions, err := a.AsS2()
+	aRegions, err := a.AsS2(geo.EmptyBehaviorError)
 	if err != nil {
 		return 0, err
 	}
-	bRegions, err := b.AsS2()
+	bRegions, err := b.AsS2(geo.EmptyBehaviorError)
 	if err != nil {
 		return 0, err
 	}

pkg/geo/geogfn/distance_test.go

@@ -11,6 +11,7 @@
 package geogfn
 
 import (
+	"fmt"
 	"math"
 	"testing"
 
@@ -270,4 +271,30 @@ func TestDistance(t *testing.T) {
 		_, err := Distance(mismatchingSRIDGeographyA, mismatchingSRIDGeographyB, UseSpheroid)
 		requireMismatchingSRIDError(t, err)
 	})
+
+	t.Run("empty geographies always error", func(t *testing.T) {
+		for _, tc := range []struct {
+			a string
+			b string
+		}{
+			{"GEOMETRYCOLLECTION EMPTY", "GEOMETRYCOLLECTION EMPTY"},
+			{"GEOMETRYCOLLECTION EMPTY", "GEOMETRYCOLLECTION (POINT(1.0 1.0), LINESTRING EMPTY)"},
+			{"POINT(1.0 1.0)", "GEOMETRYCOLLECTION (POINT(1.0 1.0), LINESTRING EMPTY)"},
+		} {
+			for _, useSphereOrSpheroid := range []UseSphereOrSpheroid{
+				UseSphere,
+				UseSpheroid,
+			} {
+				t.Run(fmt.Sprintf("Distance(%s,%s),spheroid=%t", tc.a, tc.b, useSphereOrSpheroid), func(t *testing.T) {
+					a, err := geo.ParseGeography(tc.a)
+					require.NoError(t, err)
+					b, err := geo.ParseGeography(tc.b)
+					require.NoError(t, err)
+					_, err = Distance(a, b, useSphereOrSpheroid)
+					require.Error(t, err)
+					require.True(t, geo.IsEmptyGeometryError(err))
+				})
+			}
+		}
+	})
 }

pkg/geo/geogfn/dwithin.go

@@ -19,6 +19,7 @@ import (
 )
 
 // DWithin returns whether a is within distance d of b, i.e. Distance(a, b) <= d.
+// If A or B contains empty Geography objects, this will return false.
 func DWithin(
 	a *geo.Geography, b *geo.Geography, distance float64, useSphereOrSpheroid UseSphereOrSpheroid,
 ) (bool, error) {
@@ -29,12 +30,18 @@ func DWithin(
 		return false, errors.Newf("dwithin distance cannot be less than zero")
 	}
 
-	aRegions, err := a.AsS2()
+	aRegions, err := a.AsS2(geo.EmptyBehaviorError)
 	if err != nil {
+		if geo.IsEmptyGeometryError(err) {
+			return false, nil
+		}
 		return false, err
 	}
-	bRegions, err := b.AsS2()
+	bRegions, err := b.AsS2(geo.EmptyBehaviorError)
 	if err != nil {
+		if geo.IsEmptyGeometryError(err) {
+			return false, nil
+		}
 		return false, err
 	}
 	spheroid := geographiclib.WGS84Spheroid

pkg/geo/geogfn/dwithin_test.go

@@ -106,4 +106,30 @@ func TestDWithin(t *testing.T) {
 		_, err := DWithin(geo.MustParseGeography("POINT(1.0 2.0)"), geo.MustParseGeography("POINT(3.0 4.0)"), -0.01, UseSpheroid)
 		require.Error(t, err)
 	})
+
+	t.Run("empty geographies are never dwithin each other", func(t *testing.T) {
+		for _, tc := range []struct {
+			a string
+			b string
+		}{
+			{"GEOMETRYCOLLECTION EMPTY", "GEOMETRYCOLLECTION EMPTY"},
+			{"GEOMETRYCOLLECTION EMPTY", "GEOMETRYCOLLECTION (POINT(1.0 1.0), LINESTRING EMPTY)"},
+			{"POINT(1.0 1.0)", "GEOMETRYCOLLECTION (POINT(1.0 1.0), LINESTRING EMPTY)"}, // This case errors (in a bad way) in PostGIS.
+		} {
+			for _, useSphereOrSpheroid := range []UseSphereOrSpheroid{
+				UseSphere,
+				UseSpheroid,
+			} {
+				t.Run(fmt.Sprintf("DWithin(%s,%s),spheroid=%t", tc.a, tc.b, useSphereOrSpheroid), func(t *testing.T) {
+					a, err := geo.ParseGeography(tc.a)
+					require.NoError(t, err)
+					b, err := geo.ParseGeography(tc.b)
+					require.NoError(t, err)
+					dwithin, err := DWithin(a, b, 0, useSphereOrSpheroid)
+					require.NoError(t, err)
+					require.False(t, dwithin)
+				})
+			}
+		}
+	})
 }