ST_Geogfromtext: current validation is not sufficient to ensure that the inserted Geography object(a wrapper of WKB) can legally build an S2 object

eg. for a polygon, s2 polygon needs the outer ring must contain the inner ring, but I don’t see the check in the source code.
So this will lead that some geography objects are inserted successfully into a table, but when do computation and call AsS2 method, we will find the object could not legally build an s2 object.

Hi @jievince, would you be able to provide SQL commands that can reproduce the problem? It really helps us to have a specific example, so that we can be sure we are looking at the same code and bug that you are having. Thanks!

Once you have one, you could also file it as a bug on our issue tracker.

Hi @rafiss , I mean in the file pkg/geo/geo.go:

func MakeGeographyFromGeomT(g geom.T) (Geography, error) {
	spatialObject, err := spatialObjectFromGeomT(g, geopb.SpatialObjectType_GeographyType)
	if err != nil {
		return Geography{}, err
	}
	return MakeGeography(spatialObject)
}

When constructing a geography data, it in turn calls:
MakeGeographyFromGeomT->spatialObjectFromGeomT->validateGeom.
So when inserting some geography data to a table, if validateGeom returns true, cockroach will treat it as a valid geography object and insert it to the table successfully. But validateGeom just did a simple check, so it returns true doesn’t mean it is legal to build an s2 data later. Calculating with invalid geography data will leads to undefined behavior. eg. a valid polygon needs the exterior loop contains it interior loop and this check is not done in validateGeom.

// validateGeomT validates the geom.T object across valid geom.T objects,
// returning an error if it is invalid.
func validateGeomT(t geom.T) error {
	if t.Empty() {
		return nil
	}
	switch t := t.(type) {
	case *geom.Point:
	case *geom.LineString:
		if t.NumCoords() < 2 {
			return errors.Newf("LineString must have at least 2 coordinates")
		}
	case *geom.Polygon:
		for i := 0; i < t.NumLinearRings(); i++ {
			linearRing := t.LinearRing(i)
			if linearRing.NumCoords() < 4 {
				return errors.Newf("Polygon LinearRing must have at least 4 points, found %d at position %d", linearRing.NumCoords(), i+1)
			}
			if !linearRing.Coord(0).Equal(linearRing.Layout(), linearRing.Coord(linearRing.NumCoords()-1)) {
				return errors.Newf("Polygon LinearRing at position %d is not closed", i+1)
			}
		}
	case *geom.MultiPoint:
	case *geom.MultiLineString:
		for i := 0; i < t.NumLineStrings(); i++ {
			if err := validateGeomT(t.LineString(i)); err != nil {
				return errors.Wrapf(err, "invalid MultiLineString component at position %d", i+1)
			}
		}
	case *geom.MultiPolygon:
		for i := 0; i < t.NumPolygons(); i++ {
			if err := validateGeomT(t.Polygon(i)); err != nil {
				return errors.Wrapf(err, "invalid MultiPolygon component at position %d", i+1)
			}
		}

In conclusion, there are some questions:

  1. Should we allow the invalid but accepted geography data to be stored?
    If allow it, so we need to distinguish between accepted instance and valid instance. see Polygon - SQL Server | Microsoft Docs and STIsValid (geometry Data Type) - SQL Server | Microsoft Docs. And the computation on the invalid geography data is undefined behavior. Therefore, we also need the overload st_isValid and st_makevalid for geography type.
  2. If don’t allow it, that means we just allow the data which is legal to build a s2 to be stored, could we try constructing a s2 data in validateGeom, that , if constructs failed, we report an error to the user to tell it’s illegal?

func (p *Polygon) Validate() error in s2 library : geo/polygon.go at 740aa86cb551d6388f5cf4a8f39568d52fac6ed7 · golang/geo · GitHub

Bigquery processing is simple and intuitive, which is different than the postgis and cockroachdb. If the input wkt/wkb is illegal to build an s2, it returns an null value in its functions st_geogfromtext.
See New options for BigQuery GIS geospatial data ingestion | by Michael Entin | Medium

We take the same approach to PostGIS here - we don’t validate validity of spatial objects. Users should use ST_IsValid as a CONSTRAINT if they want this to be the case (yes you’d have to cast it to geometry to compare). This is because checking the validity is expensive. As such, this is “working as intended”.

If users put invalid data in, yes they may get invalid results, but that is within the contract.

1 Like

It seems that if I want to check the validity, I need to construct an S2 data, and use its IsValid() method.
We know that cockroach’s Geography data structure is a wrapper of a wkb instead of an S2.
Let’s say If I don’t want invalid geo data to be stored in the database as BigQuery does:

  • If I insert some geography data and the column doesn’t have a gist index:
    The construction of the S2 object is wasted. It’s just used to check validity actually.
  • But if the column has a gist index, the S2 object is always needed to be constructed to get the covering cells of it and build gist index kvs. In this case, I need to make the Geography be a wrapper of a wkb and an S2 object. But this will results in higher network IO because the Geography is more complex.
    Do I understand it correctly? @otan @rafiss

i’m not sure i understand your question.

if you’re asking whether if you wanted to validate objects if you were writing an implementation, yes you could write one / a builtin using S2’s IsValid method. if we wanted to do this in CockroachDB, we’d have to convert from WKB to S2 to do so. computationally, validation is expensive - it is CPU bound (not I/O bound) - so we don’t do it for you. this also matches our aim of PostGIS compatibility, who doesn’t do validation for similar reasons.

Yes i’m writing an implementation.
I hope things don’t get too complicated, so I’m going to just provide a few core API.
I would like to do enough validation when the user inserts, which will reject any invalid data from the repository. Although it’s expensive to do that…

validation is expensive

But when the user does a calculation on the do geo column, if then want each computation result make sense, they may need call st_isvalid firstly. In this case, the expensive operation was still done, it was just postponed. (Some users can of course choose not to filter if they are willing to accept the consequences of some undefined behavior).