Merge pull request cockroachdb#63005 from mgartner/backport20.2-62893

release-20.2: opt: fix geospatial function NULL argument panics

pkg/sql/logictest/testdata/logic_test/inverted_join_geospatial

@@ -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)

pkg/sql/opt/invertedidx/geo.go

@@ -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)))
 	}
@@ -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
 		}
 	}
@@ -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)
@@ -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)))

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

@@ -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
 # --------------------------------------------------