invertedidx: harden geo code to avoid panicking

Previously, we could panic when trying to construnct an inverted
expression for a geo type. Such panics are not recovered from by the
vectorized panic-catcher (since we haven't allow-listed the package), so
it's better to propagate errors explicitly.

In particular, this allows us to prevent a crash when trying to evaluate
`st_dwithin`-like functions with infinite distance (this hits an error
in geos, so now we'll return an error, matching postgres).

Release note: None

Author: yuzefovich

pkg/sql/logictest/testdata/logic_test/inverted_index

@@ -2119,3 +2119,25 @@ c" STRING
 statement ok
 CREATE INVERTED INDEX ON t126444 ("b
 c" gin_trgm_ops)
+
+# Regression test for panicking with some GEOS errors (#151995).
+statement ok
+CREATE TABLE t151995 (
+  c0 INT PRIMARY KEY,
+  c1 GEOMETRY,
+  INVERTED INDEX (c1 ASC)
+);
+
+statement ok
+INSERT INTO t151995 (c0, c1) VALUES (
+        0,
+        '010300004001000000040000004ED667271F45EEC180F702131A41A7C138EDC3641D46F041E9E58A67A768F0C1CBB3A399C439FEC1189C38E3BBDDEEC16074F4D5F0C7FA414CCB652363EC01429029CED0E787E1C14ED667271F45EEC180F702131A41A7C138EDC3641D46F041':::GEOMETRY
+       );
+
+statement ok
+ANALYZE t151995;
+
+statement error geos error: IllegalArgumentException: BufferOp::getResultGeometry distance must be a finite value
+SELECT *
+  FROM t151995 AS t1
+  JOIN t151995 AS t2 ON st_dfullywithinexclusive(t2.c1, t1.c1, '+Inf');

pkg/sql/opt/invertedidx/geo.go

@@ -61,7 +61,7 @@ func GetGeoIndexRelationship(expr opt.ScalarExpr) (_ geoindex.RelationshipType,
 // and getSpanExprForGeometryIndex and used in extractGeoFilterCondition.
 type getSpanExprForGeoIndexFn func(
 	context.Context, tree.Datum, []tree.Datum, geoindex.RelationshipType, geopb.Config,
-) inverted.Expression
+) (inverted.Expression, error)
 
 // getSpanExprForGeographyIndex gets a SpanExpression that constrains the given
 // geography index according to the given constant and geospatial relationship.
@@ -71,67 +71,67 @@ func getSpanExprForGeographyIndex(
 	additionalParams []tree.Datum,
 	relationship geoindex.RelationshipType,
 	indexConfig geopb.Config,
-) inverted.Expression {
+) (inverted.Expression, error) {
 	geogIdx := geoindex.NewS2GeographyIndex(*indexConfig.S2Geography)
 	geog := d.(*tree.DGeography).Geography
 
 	switch relationship {
 	case geoindex.Covers:
 		unionKeySpans, err := geogIdx.Covers(ctx, geog)
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
-		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
+		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans), nil
 
 	case geoindex.CoveredBy:
 		rpKeyExpr, err := geogIdx.CoveredBy(ctx, geog)
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
 		spanExpr, err := invertedexpr.GeoRPKeyExprToSpanExpr(rpKeyExpr)
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
-		return spanExpr
+		return spanExpr, nil
 
 	case geoindex.DWithin:
 		// Parameters are type checked earlier. Keep this consistent with the definition
 		// in geo_builtins.go.
 		if len(additionalParams) != 1 && len(additionalParams) != 2 {
-			panic(errors.AssertionFailedf("unexpected param length %d", len(additionalParams)))
+			return nil, errors.AssertionFailedf("unexpected param length %d", len(additionalParams))
 		}
 		d, ok := additionalParams[0].(*tree.DFloat)
 		if !ok {
-			panic(errors.AssertionFailedf(
-				"parameter %s is not float", additionalParams[0].ResolvedType()))
+			return nil, errors.AssertionFailedf(
+				"parameter %s is not float", additionalParams[0].ResolvedType())
 		}
 		distanceMeters := float64(*d)
 		useSpheroid := geogfn.UseSpheroid
 		if len(additionalParams) == 2 {
 			b, ok := additionalParams[1].(*tree.DBool)
 			if !ok {
-				panic(errors.AssertionFailedf(
-					"parameter %s is not bool", additionalParams[1].ResolvedType()))
+				return nil, errors.AssertionFailedf(
+					"parameter %s is not bool", additionalParams[1].ResolvedType())
 			}
 			if !*b {
 				useSpheroid = geogfn.UseSphere
 			}
 		}
 		unionKeySpans, err := geogIdx.DWithin(ctx, geog, distanceMeters, useSpheroid)
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
-		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
+		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans), nil
 
 	case geoindex.Intersects:
 		unionKeySpans, err := geogIdx.Intersects(ctx, geog)
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
-		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
+		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans), nil
 
 	default:
-		panic(errors.AssertionFailedf("unhandled relationship: %v", relationship))
+		return nil, errors.AssertionFailedf("unhandled relationship: %v", relationship)
 	}
 }
 
@@ -158,54 +158,54 @@ func getSpanExprForGeometryIndex(
 	additionalParams []tree.Datum,
 	relationship geoindex.RelationshipType,
 	indexConfig geopb.Config,
-) inverted.Expression {
+) (inverted.Expression, error) {
 	geomIdx := geoindex.NewS2GeometryIndex(*indexConfig.S2Geometry)
 	geom := d.(*tree.DGeometry).Geometry
 
 	switch relationship {
 	case geoindex.Covers:
 		unionKeySpans, err := geomIdx.Covers(ctx, geom)
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
-		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
+		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans), nil
 
 	case geoindex.CoveredBy:
 		rpKeyExpr, err := geomIdx.CoveredBy(ctx, geom)
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
 		spanExpr, err := invertedexpr.GeoRPKeyExprToSpanExpr(rpKeyExpr)
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
-		return spanExpr
+		return spanExpr, nil
 
 	case geoindex.DFullyWithin:
 		distance := getDistanceParam(additionalParams)
 		unionKeySpans, err := geomIdx.DFullyWithin(ctx, geom, distance)
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
-		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
+		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans), nil
 
 	case geoindex.DWithin:
 		distance := getDistanceParam(additionalParams)
 		unionKeySpans, err := geomIdx.DWithin(ctx, geom, distance)
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
-		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
+		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans), nil
 
 	case geoindex.Intersects:
 		unionKeySpans, err := geomIdx.Intersects(ctx, geom)
 		if err != nil {
-			panic(err)
+			return nil, err
 		}
-		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
+		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans), nil
 
 	default:
-		panic(errors.AssertionFailedf("unhandled relationship: %v", relationship))
+		return nil, errors.AssertionFailedf("unhandled relationship: %v", relationship)
 	}
 }
 
@@ -630,7 +630,11 @@ func extractGeoFilterCondition(
 	preFilterExpr :=
 		makeExprFromRelationshipAndParams(factory, expr, args, commuteArgs, relationship)
 
-	return getSpanExpr(ctx, d, additionalParams, relationship, index.GeoConfig()),
+	invExpr, err := getSpanExpr(ctx, d, additionalParams, relationship, index.GeoConfig())
+	if err != nil {
+		return inverted.NonInvertedColExpression{}, nil
+	}
+	return invExpr,
 		&invertedexpr.PreFiltererStateForInvertedFilterer{
 			Expr: preFilterExpr,
 			Col:  arg2.Col,
@@ -1022,7 +1026,11 @@ func NewGeoDatumsToInvertedExpr(
 			// it for every row.
 			var invertedExpr inverted.Expression
 			if d, ok := nonIndexParam.(tree.Datum); ok {
-				invertedExpr = g.getSpanExpr(ctx, d, additionalParams, relationship, g.indexConfig)
+				var err error
+				invertedExpr, err = g.getSpanExpr(ctx, d, additionalParams, relationship, g.indexConfig)
+				if err != nil {
+					return nil, err
+				}
 			} else if funcExprCount == 1 {
 				// Currently pre-filtering is limited to a single FuncExpr.
 				preFilterRelationship = relationship
@@ -1079,7 +1087,7 @@ func (g *geoDatumsToInvertedExpr) Convert(
 			if g.filterer != nil {
 				preFilterState = g.filterer.Bind(d)
 			}
-			return g.getSpanExpr(ctx, d, t.additionalParams, t.relationship, g.indexConfig), nil
+			return g.getSpanExpr(ctx, d, t.additionalParams, t.relationship, g.indexConfig)
 
 		default:
 			return nil, fmt.Errorf("unsupported expression %v", t)