diff --git a/pkg/sql/opt/invertedexpr/BUILD.bazel b/pkg/sql/opt/invertedexpr/BUILD.bazel index 7fd01c24701a..4e16a7ccf61c 100644 --- a/pkg/sql/opt/invertedexpr/BUILD.bazel +++ b/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", ], diff --git a/pkg/sql/opt/invertedexpr/geo_expression.go b/pkg/sql/opt/invertedexpr/geo_expression.go index b84dd42d2ee6..5743f4321e0d 100644 --- a/pkg/sql/opt/invertedexpr/geo_expression.go +++ b/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 diff --git a/pkg/sql/opt/invertedexpr/geo_expression_test.go b/pkg/sql/opt/invertedexpr/geo_expression_test.go index 2347ac479a82..65b4c71948ad 100644 --- a/pkg/sql/opt/invertedexpr/geo_expression_test.go +++ b/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: " + "factored_union_spans: > ", }, - { - uks: nil, - expected: "", - }, } 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: "", - }, { // 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) } diff --git a/pkg/sql/opt/invertedidx/geo.go b/pkg/sql/opt/invertedidx/geo.go index 2ac778698ea5..849b97f1733a 100644 --- a/pkg/sql/opt/invertedidx/geo.go +++ b/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,10 +73,9 @@ 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: @@ -84,16 +83,18 @@ func getSpanExprForGeographyIndex( 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,10 +159,9 @@ 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: @@ -171,16 +169,18 @@ func getSpanExprForGeometryIndex( 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) @@ -188,7 +188,7 @@ func getSpanExprForGeometryIndex( if err != nil { panic(err) } - spanExpr = invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans) + return invertedexpr.GeoUnionKeySpansToSpanExpr(unionKeySpans) case geoindex.DWithin: distance := getDistanceParam(additionalParams) @@ -196,20 +196,18 @@ func getSpanExprForGeometryIndex( 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 { diff --git a/pkg/sql/opt/xform/testdata/rules/select b/pkg/sql/opt/xform/testdata/rules/select index 22d2dcd6b172..3c5fd198ad18 100644 --- a/pkg/sql/opt/xform/testdata/rules/select +++ b/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 # --------------------------------------------------