Merge #62893 #62984

62893: opt: fix geospatial function NULL argument panics r=mgartner a=mgartner

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 #60527
Fixes #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.


62984: storage: reduce memory allocations in intentInterleavingIter r=sumeerbhola a=sumeerbhola

The memory allocations for computing the bounds for the
EngineIterator were 33% of the memory allocations in
batcheval.Scan in a joinReader benchmark (the rest, as
expected, is in pebbleResults for storing the result
of the scan). These allocations were 6% of the cpu in
MVCCScanToBytes in the same benchmark.

Release note: None

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: sumeerbhola <[email protected]>

pkg/sql/logictest/testdata/logic_test/inverted_join_geospatial

@@ -477,3 +477,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

@@ -139,8 +139,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)))
 	}
@@ -456,6 +457,11 @@ func extractInfoFromExpr(
 		arg1, exprArg2 = exprArg2, arg1
 	}
 
+	// The first argument must be non-NULL.
+	if arg1.Op() == opt.NullOp {
+		return 0, nil, nil, nil, false
+	}
+
 	// The second argument should be a variable corresponding to the index
 	// column.
 	arg2, ok = exprArg2.(*memo.VariableExpr)
@@ -467,9 +473,10 @@ func extractInfoFromExpr(
 		return 0, nil, nil, nil, false
 	}
 
-	// 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 0, nil, nil, nil, false
 		}
 		additionalParams = append(additionalParams, memo.ExtractConstDatum(args.Child(i)))

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

@@ -4823,6 +4823,53 @@ select
  └── filters
       └── st_intersects(g:2, '0103000020E610000000000000') [outer=(2), immutable, constraints=(/2: (/NULL - ])]
 
+# 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!null
+ ├── immutable
+ ├── scan t60527
+ │    └── columns: g:1
+ └── filters
+      ├── st_covers(CAST(NULL AS GEOGRAPHY), g:1) [outer=(1), immutable, constraints=(/1: (/NULL - ])]
+      └── st_dwithin(CAST(NULL AS GEOGRAPHY), g:1, 1.0) [outer=(1), immutable, constraints=(/1: (/NULL - ])]
+
+# 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!null
+ ├── immutable
+ ├── scan t62686
+ │    └── columns: c:1
+ └── filters
+      └── st_dfullywithin(c:1, '0101000000000000000000F03F000000000000F03F', CAST(NULL AS FLOAT8)) [outer=(1), immutable, constraints=(/1: (/NULL - ])]
+
 # --------------------------------------------------
 # GenerateZigzagJoins
 # --------------------------------------------------

pkg/storage/intent_interleaving_iter.go

@@ -123,7 +123,10 @@ type intentInterleavingIter struct {
 	valid bool
 	err   error
 
-	intentKeyBuf []byte
+	// Buffers to reuse memory when constructing lock table keys for bounds and
+	// seeks.
+	intentKeyBuf      []byte
+	intentLimitKeyBuf []byte
 }
 
 var _ MVCCIterator = &intentInterleavingIter{}
@@ -190,25 +193,29 @@ func newIntentInterleavingIterator(reader Reader, opts IterOptions) MVCCIterator
 	// bound for prefix iteration, though since they don't need to, most callers
 	// don't.
 
+	iiIter := intentInterleavingIterPool.Get().(*intentInterleavingIter)
 	intentOpts := opts
-	var intentKeyBuf []byte
+	intentKeyBuf := iiIter.intentKeyBuf
+	intentLimitKeyBuf := iiIter.intentLimitKeyBuf
 	if opts.LowerBound != nil {
-		intentOpts.LowerBound, intentKeyBuf = keys.LockTableSingleKey(opts.LowerBound, nil)
+		intentOpts.LowerBound, intentKeyBuf = keys.LockTableSingleKey(opts.LowerBound, intentKeyBuf)
 	} else if !opts.Prefix {
 		// Make sure we don't step outside the lock table key space. Note that
 		// this is the case where the lower bound was not set and
 		// constrainedToLocal.
 		intentOpts.LowerBound = keys.LockTableSingleKeyStart
 	}
 	if opts.UpperBound != nil {
-		intentOpts.UpperBound, _ = keys.LockTableSingleKey(opts.UpperBound, nil)
+		intentOpts.UpperBound, intentLimitKeyBuf =
+			keys.LockTableSingleKey(opts.UpperBound, intentLimitKeyBuf)
 	} else if !opts.Prefix {
 		// Make sure we don't step outside the lock table key space. Note that
 		// this is the case where the upper bound was not set and
 		// constrainedToGlobal.
 		intentOpts.UpperBound = keys.LockTableSingleKeyEnd
 	}
-	// Note that we can reuse intentKeyBuf after NewEngineIterator returns.
+	// Note that we can reuse intentKeyBuf, intentLimitKeyBuf after
+	// NewEngineIterator returns.
 	intentIter := reader.NewEngineIterator(intentOpts)
 
 	// The creation of these iterators can race with concurrent mutations, which
@@ -221,13 +228,15 @@ func newIntentInterleavingIterator(reader Reader, opts IterOptions) MVCCIterator
 	} else {
 		iter = newMVCCIteratorByCloningEngineIter(intentIter, opts)
 	}
-	iiIter := intentInterleavingIterPool.Get().(*intentInterleavingIter)
+
 	*iiIter = intentInterleavingIter{
-		prefix:       opts.Prefix,
-		constraint:   constraint,
-		iter:         iter,
-		intentIter:   intentIter,
-		intentKeyBuf: intentKeyBuf,
+		prefix:                               opts.Prefix,
+		constraint:                           constraint,
+		iter:                                 iter,
+		intentIter:                           intentIter,
+		intentKeyAsNoTimestampMVCCKeyBacking: iiIter.intentKeyAsNoTimestampMVCCKeyBacking,
+		intentKeyBuf:                         intentKeyBuf,
+		intentLimitKeyBuf:                    intentLimitKeyBuf,
 	}
 	return iiIter
 }
@@ -610,7 +619,11 @@ func (i *intentInterleavingIter) Value() []byte {
 func (i *intentInterleavingIter) Close() {
 	i.iter.Close()
 	i.intentIter.Close()
-	*i = intentInterleavingIter{}
+	*i = intentInterleavingIter{
+		intentKeyAsNoTimestampMVCCKeyBacking: i.intentKeyAsNoTimestampMVCCKeyBacking,
+		intentKeyBuf:                         i.intentKeyBuf,
+		intentLimitKeyBuf:                    i.intentLimitKeyBuf,
+	}
 	intentInterleavingIterPool.Put(i)
 }