Merge #60598

60598: opt: fix panic in GenerateInvertedIndexScans r=mgartner a=mgartner

This commit fixes a nil-pointer exception caused when empty index spans
were generated from a filter on an inverted column.

Fixes #59702

Release note (bug fix): A bug has been fixed that caused errors for some
queries on tables with GEOMETRY or GEOGRAPHY inverted indexes with
filters containing shapes with zero area.

Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>

pkg/sql/opt/invertedexpr/BUILD.bazel

@@ -28,6 +28,7 @@ go_test(
     embed = [":invertedexpr"],
     deps = [
         "//pkg/geo/geoindex",
+        "//pkg/sql/inverted",
         "//pkg/util/leaktest",
         "@com_github_stretchr_testify//require",
     ],

pkg/sql/opt/invertedexpr/geo_expression.go

@@ -61,9 +61,9 @@ func geoToSpan(span geoindex.KeySpan, b []byte) (inverted.Span, []byte) {
 
 // GeoUnionKeySpansToSpanExpr converts geoindex.UnionKeySpans to a
 // SpanExpression.
-func GeoUnionKeySpansToSpanExpr(ukSpans geoindex.UnionKeySpans) *inverted.SpanExpression {
+func GeoUnionKeySpansToSpanExpr(ukSpans geoindex.UnionKeySpans) inverted.Expression {
 	if len(ukSpans) == 0 {
-		return nil
+		return inverted.NonInvertedColExpression{}
 	}
 	// Avoid per-span heap allocations. Each of the 2 keys in a span is the
 	// geoInvertedIndexMarker (1 byte) followed by a varint.
@@ -79,9 +79,9 @@ func GeoUnionKeySpansToSpanExpr(ukSpans geoindex.UnionKeySpans) *inverted.SpanEx
 }
 
 // GeoRPKeyExprToSpanExpr converts geoindex.RPKeyExpr to SpanExpression.
-func GeoRPKeyExprToSpanExpr(rpExpr geoindex.RPKeyExpr) (*inverted.SpanExpression, error) {
+func GeoRPKeyExprToSpanExpr(rpExpr geoindex.RPKeyExpr) (inverted.Expression, error) {
 	if len(rpExpr) == 0 {
-		return nil, nil
+		return inverted.NonInvertedColExpression{}, nil
 	}
 	spansToRead := make([]inverted.Span, 0, len(rpExpr))
 	var b []byte // avoid per-expr heap allocations
@@ -128,7 +128,7 @@ func GeoRPKeyExprToSpanExpr(rpExpr geoindex.RPKeyExpr) (*inverted.SpanExpression
 		}
 	}
 	if len(stack) != 1 {
-		return nil, errors.Errorf("malformed expression: %s", rpExpr)
+		return inverted.NonInvertedColExpression{}, errors.Errorf("malformed expression: %s", rpExpr)
 	}
 	spanExpr := *stack[0]
 	spanExpr.SpansToRead = spansToRead

pkg/sql/opt/invertedexpr/geo_expression_test.go

@@ -14,6 +14,7 @@ import (
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/geo/geoindex"
+	"github.com/cockroachdb/cockroach/pkg/sql/inverted"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/stretchr/testify/require"
 )
@@ -36,14 +37,15 @@ func TestUnionKeySpansToSpanExpr(t *testing.T) {
 				"factored_union_spans:<start:\"B\\222\" end:\"B\\223\" > " +
 				"factored_union_spans:<start:\"B\\211\" end:\"B\\214\" > > ",
 		},
-		{
-			uks:      nil,
-			expected: "<nil>",
-		},
 	}
 	for _, c := range cases {
-		require.Equal(t, c.expected, GeoUnionKeySpansToSpanExpr(c.uks).ToProto().String())
+		spanExpr := GeoUnionKeySpansToSpanExpr(c.uks).(*inverted.SpanExpression)
+		require.Equal(t, c.expected, spanExpr.ToProto().String())
 	}
+
+	// Test with nil union key spans.
+	expr := GeoUnionKeySpansToSpanExpr(nil)
+	require.Equal(t, inverted.NonInvertedColExpression{}, expr)
 }
 
 func TestRPKeyExprToSpanExpr(t *testing.T) {
@@ -55,10 +57,6 @@ func TestRPKeyExprToSpanExpr(t *testing.T) {
 		err      string
 	}
 	cases := []testCase{
-		{
-			rpx:      nil,
-			expected: "<nil>",
-		},
 		{
 			// Union of two keys.
 			rpx: []geoindex.RPExprElement{geoindex.Key(5), geoindex.Key(10), geoindex.RPSetUnion},
@@ -125,9 +123,14 @@ func TestRPKeyExprToSpanExpr(t *testing.T) {
 		rpx, err := GeoRPKeyExprToSpanExpr(c.rpx)
 		if len(c.err) == 0 {
 			require.NoError(t, err)
-			require.Equal(t, c.expected, rpx.ToProto().String())
+			require.Equal(t, c.expected, rpx.(*inverted.SpanExpression).ToProto().String())
 		} else {
 			require.Equal(t, c.err, err.Error())
 		}
 	}
+
+	// Test with nil RPKeyExpr.
+	expr, err := GeoRPKeyExprToSpanExpr(nil)
+	require.NoError(t, err)
+	require.Equal(t, inverted.NonInvertedColExpression{}, expr)
 }

pkg/sql/opt/invertedidx/geo.go

@@ -63,7 +63,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, *geoindex.Config,
-) *inverted.SpanExpression
+) inverted.Expression
 
 // getSpanExprForGeographyIndex gets a SpanExpression that constrains the given
 // geography index according to the given constant and geospatial relationship.
@@ -73,27 +73,28 @@ func getSpanExprForGeographyIndex(
 	additionalParams []tree.Datum,
 	relationship geoindex.RelationshipType,
 	indexConfig *geoindex.Config,
-) *inverted.SpanExpression {
+) inverted.Expression {
 	geogIdx := geoindex.NewS2GeographyIndex(*indexConfig.S2Geography)
 	geog := d.(*tree.DGeography).Geography
-	var spanExpr *inverted.SpanExpression
 
 	switch relationship {
 	case geoindex.Covers:
 		unionKeySpans, err := geogIdx.Covers(ctx, geog)
 		if err != nil {
 			panic(err)
 		}
-		spanExpr = invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
+		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
 
 	case geoindex.CoveredBy:
 		rpKeyExpr, err := geogIdx.CoveredBy(ctx, geog)
 		if err != nil {
 			panic(err)
 		}
-		if spanExpr, err = invertedexpr.GeoRPKeyExprToSpanExpr(rpKeyExpr); err != nil {
+		spanExpr, err := invertedexpr.GeoRPKeyExprToSpanExpr(rpKeyExpr)
+		if err != nil {
 			panic(err)
 		}
+		return spanExpr
 
 	case geoindex.DWithin:
 		// Parameters are type checked earlier. Keep this consistent with the definition
@@ -122,20 +123,18 @@ func getSpanExprForGeographyIndex(
 		if err != nil {
 			panic(err)
 		}
-		spanExpr = invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
+		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
 
 	case geoindex.Intersects:
 		unionKeySpans, err := geogIdx.Intersects(ctx, geog)
 		if err != nil {
 			panic(err)
 		}
-		spanExpr = invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
+		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
 
 	default:
 		panic(errors.AssertionFailedf("unhandled relationship: %v", relationship))
 	}
-
-	return spanExpr
 }
 
 // Helper for DWithin and DFullyWithin.
@@ -160,56 +159,55 @@ func getSpanExprForGeometryIndex(
 	additionalParams []tree.Datum,
 	relationship geoindex.RelationshipType,
 	indexConfig *geoindex.Config,
-) *inverted.SpanExpression {
+) inverted.Expression {
 	geomIdx := geoindex.NewS2GeometryIndex(*indexConfig.S2Geometry)
 	geom := d.(*tree.DGeometry).Geometry
-	var spanExpr *inverted.SpanExpression
 
 	switch relationship {
 	case geoindex.Covers:
 		unionKeySpans, err := geomIdx.Covers(ctx, geom)
 		if err != nil {
 			panic(err)
 		}
-		spanExpr = invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
+		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
 
 	case geoindex.CoveredBy:
 		rpKeyExpr, err := geomIdx.CoveredBy(ctx, geom)
 		if err != nil {
 			panic(err)
 		}
-		if spanExpr, err = invertedexpr.GeoRPKeyExprToSpanExpr(rpKeyExpr); err != nil {
+		spanExpr, err := invertedexpr.GeoRPKeyExprToSpanExpr(rpKeyExpr)
+		if err != nil {
 			panic(err)
 		}
+		return spanExpr
 
 	case geoindex.DFullyWithin:
 		distance := getDistanceParam(additionalParams)
 		unionKeySpans, err := geomIdx.DFullyWithin(ctx, geom, distance)
 		if err != nil {
 			panic(err)
 		}
-		spanExpr = invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
+		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
 
 	case geoindex.DWithin:
 		distance := getDistanceParam(additionalParams)
 		unionKeySpans, err := geomIdx.DWithin(ctx, geom, distance)
 		if err != nil {
 			panic(err)
 		}
-		spanExpr = invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
+		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
 
 	case geoindex.Intersects:
 		unionKeySpans, err := geomIdx.Intersects(ctx, geom)
 		if err != nil {
 			panic(err)
 		}
-		spanExpr = invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
+		return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans)
 
 	default:
 		panic(errors.AssertionFailedf("unhandled relationship: %v", relationship))
 	}
-
-	return spanExpr
 }
 
 type geoJoinPlanner struct {
@@ -525,9 +523,9 @@ type geoInvertedExpr struct {
 	nonIndexParam    tree.TypedExpr
 	additionalParams []tree.Datum
 
-	// spanExpr is the result of evaluating the geospatial relationship
+	// invertedExpr is the result of evaluating the geospatial relationship
 	// represented by this geoInvertedExpr. It is nil prior to evaluation.
-	spanExpr *inverted.SpanExpression
+	invertedExpr inverted.Expression
 }
 
 var _ tree.TypedExpr = &geoInvertedExpr{}
@@ -803,9 +801,9 @@ func NewGeoDatumsToInvertedExpr(
 
 			// If possible, get the span expression now so we don't need to recompute
 			// it for every row.
-			var spanExpr *inverted.SpanExpression
+			var invertedExpr inverted.Expression
 			if d, ok := nonIndexParam.(tree.Datum); ok {
-				spanExpr = g.getSpanExpr(evalCtx.Ctx(), d, additionalParams, relationship, g.indexConfig)
+				invertedExpr = g.getSpanExpr(evalCtx.Ctx(), d, additionalParams, relationship, g.indexConfig)
 			} else if funcExprCount == 1 {
 				// Currently pre-filtering is limited to a single FuncExpr.
 				preFilterRelationship = relationship
@@ -817,7 +815,7 @@ func NewGeoDatumsToInvertedExpr(
 				relationship:     relationship,
 				nonIndexParam:    nonIndexParam,
 				additionalParams: additionalParams,
-				spanExpr:         spanExpr,
+				invertedExpr:     invertedExpr,
 			}, nil
 
 		default:
@@ -848,9 +846,9 @@ func (g *geoDatumsToInvertedExpr) Convert(
 	evalInvertedExprLeaf := func(expr tree.TypedExpr) (inverted.Expression, error) {
 		switch t := expr.(type) {
 		case *geoInvertedExpr:
-			if t.spanExpr != nil {
+			if t.invertedExpr != nil {
 				// We call Copy so the caller can modify the returned expression.
-				return t.spanExpr.Copy(), nil
+				return t.invertedExpr.Copy(), nil
 			}
 			d, err := t.nonIndexParam.Eval(g.evalCtx)
 			if err != nil {

pkg/sql/opt/xform/testdata/rules/select

@@ -4092,6 +4092,31 @@ exec-ddl
 DROP INDEX mc_idx
 ----
 
+# Regression test for #59702. Do not panic when empty index spans are generated
+# from a filter on an inverted column.
+exec-ddl
+CREATE TABLE t59702 (
+  k INT PRIMARY KEY,
+  g GEOGRAPHY,
+  INVERTED INDEX (g)
+)
+----
+
+opt
+SELECT * FROM t59702 WHERE st_intersects(g, st_buffer(st_makepoint(1, 1)::GEOGRAPHY, 0))
+----
+select
+ ├── columns: k:1!null g:2!null
+ ├── immutable
+ ├── key: (1)
+ ├── fd: (1)-->(2)
+ ├── scan t59702
+ │    ├── columns: k:1!null g:2
+ │    ├── key: (1)
+ │    └── fd: (1)-->(2)
+ └── filters
+      └── st_intersects(g:2, '0103000020E610000000000000') [outer=(2), immutable, constraints=(/2: (/NULL - ])]
+
 # --------------------------------------------------
 # GenerateZigzagJoins
 # --------------------------------------------------