Skip to content

Commit

Permalink
opt: fix geospatial function NULL argument panics
Browse files Browse the repository at this point in the history
This commit fixes panics that occurred when planning inverted index
scans and when performing inverted joins on `GEOMETRY` and `GEOGRAPHY`
inverted indexes for queries containing geospatial functions with an
`NULL` constant argument.

This panic only occurred when the constant `NULL` argument was cast to
an acceptable type for the function overload, for example
`ST_DWithin(a, b, NULL::FLOAT8)` or `ST_DWithin(NULL:GEOMETRY, b, 1)`.
Without the cast, no panic occured because `tree.FuncExpr.TypeCheck`
rewrites a `tree.FuncExpr` as `tree.DNull` when any of the arguments
have an unknown type, such as an uncast `NULL`. This rewriting happens
even before the `tree.FuncExpr` is built into a `memo.FunctionExpr` by
optbuilder.

Fixes cockroachdb#60527
Fixes cockroachdb#62686

Release note (bug fix): Queries that reference tables with `GEOMETRY` or
`GEOGRAPHY` inverted indexes and that call geospatial functions with
constant `NULL` values cast to a type, like `NULL::GEOMETRY` or
`NULL::FLOAT8`, no longer error. This bug was present since 20.2.
  • Loading branch information
mgartner committed Apr 2, 2021
1 parent 1eb268e commit af72a3e
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 6 deletions.
12 changes: 12 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/inverted_join_geospatial
Original file line number Diff line number Diff line change
Expand Up @@ -415,3 +415,15 @@ FULL OUTER JOIN
ON inv_join.k1 = cross_join.k1 AND inv_join.k2 = cross_join.k2
WHERE inv_join.k1 IS NULL OR cross_join.k1 IS NULL
----

# Regression test for #62686. An inverted join with a geospatial function and a
# NULL distance argument should not error.
statement ok
CREATE TABLE t62686 (
c GEOMETRY,
INVERTED INDEX (c ASC)
);
INSERT INTO t62686 VALUES (ST_GeomFromText('POINT(1 1)'));

statement ok
SELECT * FROM t62686 t1 JOIN t62686 t2 ON ST_DFullyWithin(t1.c, t2.c, NULL::FLOAT8)
20 changes: 14 additions & 6 deletions pkg/sql/opt/invertedidx/geo.go
Original file line number Diff line number Diff line change
Expand Up @@ -235,8 +235,9 @@ func getSpanExprForGeographyIndex(

// Helper for DWithin and DFullyWithin.
func getDistanceParam(params []tree.Datum) float64 {
// Parameters are type checked earlier. Keep this consistent with the definition
// in geo_builtins.go.
// Parameters are type checked earlier when the expression is built by
// optbuilder. extractInfoFromExpr ensures that the parameters are non-NULL
// constants. Keep this consistent with the definition in geo_builtins.go.
if len(params) != 1 {
panic(errors.AssertionFailedf("unexpected param length %d", len(params)))
}
Expand Down Expand Up @@ -447,9 +448,10 @@ func joinGeoIndexFromExpr(
return nil
}

// Any additional params must be constant.
// Any additional params must be non-NULL constants.
for i := 2; i < args.ChildCount(); i++ {
if !memo.CanExtractConstDatum(args.Child(i)) {
arg := args.Child(i)
if arg.Op() == opt.NullOp || !memo.CanExtractConstDatum(arg) {
return nil
}
}
Expand Down Expand Up @@ -603,6 +605,11 @@ func constrainGeoIndexFromExpr(
}
d := memo.ExtractConstDatum(arg1)

// The first argument must be non-NULL.
if arg1.Op() == opt.NullOp {
return invertedexpr.NonInvertedColExpression{}
}

// The second argument should be a variable corresponding to the index
// column.
variable, ok := arg2.(*memo.VariableExpr)
Expand All @@ -614,10 +621,11 @@ func constrainGeoIndexFromExpr(
return invertedexpr.NonInvertedColExpression{}
}

// Any additional params must be constant.
// Any additional params must be non-NULL constants.
var additionalParams []tree.Datum
for i := 2; i < args.ChildCount(); i++ {
if !memo.CanExtractConstDatum(args.Child(i)) {
arg := args.Child(i)
if arg.Op() == opt.NullOp || !memo.CanExtractConstDatum(arg) {
return invertedexpr.NonInvertedColExpression{}
}
additionalParams = append(additionalParams, memo.ExtractConstDatum(args.Child(i)))
Expand Down
47 changes: 47 additions & 0 deletions pkg/sql/opt/xform/testdata/rules/select
Original file line number Diff line number Diff line change
Expand Up @@ -2402,6 +2402,53 @@ select
└── filters
└── st_intersects(g:2, '0103000020E610000000000000') [outer=(2), immutable]

# Regression test for #60527. Do not panic when a geospatial function has a NULL
# GEOMETRY/GEOGRAPHY argument.
exec-ddl
CREATE TABLE t60527 (
g GEOGRAPHY,
INVERTED INDEX (g)
)
----

# TODO(mgartner): Functions with NullableArgs=false and a NULL argument can be
# normalized to NULL. This would simplify this query plan to an empty Values
# expression.
opt
SELECT * FROM t60527 WHERE (ST_Covers(NULL::GEOGRAPHY, g) AND ST_DWithin(NULL::GEOGRAPHY, g, 1))
----
select
├── columns: g:1
├── immutable
├── scan t60527
│ └── columns: g:1
└── filters
├── st_covers(CAST(NULL AS GEOGRAPHY), g:1) [outer=(1), immutable]
└── st_dwithin(CAST(NULL AS GEOGRAPHY), g:1, 1.0) [outer=(1), immutable]

# Regression test for #62686. Do not panic when a geospatial function has a NULL
# additional argument.
exec-ddl
CREATE TABLE t62686 (
c GEOMETRY NULL,
INVERTED INDEX (c ASC)
)
----

# TODO(mgartner): Functions with NullableArgs=false and a NULL argument can be
# normalized to NULL. This would simplify this query plan to an empty Values
# expression.
opt
SELECT * FROM t62686 WHERE ST_DFullyWithin(c, ST_GeomFromText('POINT(1 1)'), NULL::FLOAT8)
----
select
├── columns: c:1
├── immutable
├── scan t62686
│ └── columns: c:1
└── filters
└── st_dfullywithin(c:1, '0101000000000000000000F03F000000000000F03F', CAST(NULL AS FLOAT8)) [outer=(1), immutable]

# --------------------------------------------------
# SplitDisjunction
# --------------------------------------------------
Expand Down

0 comments on commit af72a3e

Please sign in to comment.