opt: fix geospatial function NULL argument panics

[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.

[cockroach-teamcity]

This change is 

[RaduBerinde]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rytaft and @sumeerbhola)

[rytaft]

Awesome! I think this should fix #60527 too. Does that seem right?

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @sumeerbhola)

[rytaft]

Actually, I don't think this would fix that issue. The reproduction I just found is:

CREATE TABLE t (g GEOGRAPHY, INVERTED INDEX (g));
SELECT * FROM t WHERE (st_covers(NULL::GEOGRAPHY, g) AND st_dwithin(NULL::GEOGRAPHY, g, 1));

If it's easy to fix that one in this PR please do, otherwise I'll fix it in a separate PR.

[mgartner]

If it's easy to fix that one in this PR please do, otherwise I'll fix it in a separate PR.

I'll give it a shot.

[mgartner]

@rytaft PTAL. I couldn't produce a panic with a NULL::GEOGRAPHY in an inverted join, so there's not regression test for it.

[rytaft]

thanks!!

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @sumeerbhola)

[sumeerbhola]

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.

I'm confused: makeSTDWithinBuiltin does not set NullableArgs to true, so why is NULL::FLOAT8 or NULL:GEOGRAPHY being passed through. And the optimizer plan in the new datadriven tests suggest that the ST_DWithin builtin will actually be called, when it isn't equipped to handle these null args (it does things like dist := tree.MustBeDFloat(args[2]) followed by float64(dist)).

Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner)

[mgartner]

I'm confused: makeSTDWithinBuiltin does not set NullableArgs to true, so why is NULL::FLOAT8 or NULL:GEOGRAPHY being passed through.

I'm not sure what you mean by "being passed through", but this might clear things up:

When there is a function with NullableArgs=false and a constant untyped NULL argument, type-checking code run during the optbuild phase rewrites that function to tree.DNull here. When the NULL is typed (type is not unknown), this rewrite does not occur. (I'm hoping this rewrite can be removed in favor of a normalization rule that I want to add: #62924).

And the optimizer plan in the new datadriven tests suggest that the ST_DWithin builtin will actually be called, when it isn't equipped to handle these null args (it does things like dist := tree.MustBeDFloat(args[2]) followed by float64(dist)).

I'm not 100% sure, but it appears that we avoid evaluating a function at all if NullableArgs=false and one of its arguments is NULL. That logic appears to take place here. Therefore, the dist := tree.MustBeDFloat(args[2]) code never executes.

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner)

[mgartner]

When the NULL is typed (type is not unknown), this rewrite does not occur.

To clarify, the NULL is typed when there is a cast, like NULL::FLOAT8.

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner)

[sumeerbhola]

Let me replay this back: with a constant untyped NULL argument we rewrite the function to tree.DNull in the optimizer when type checking, but when it is a typed NULL we wait until execution time in eval.go to skip the evaluation -- is that accurate?

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner)

---

pkg/sql/logictest/testdata/logic_test/inverted_join_geospatial, line 491 at r2 (raw file):

statement ok
SELECT * FROM t62686 t1 JOIN t62686 t2 ON ST_DFullyWithin(t1.c, t2.c, NULL::FLOAT8)

what plan is generated for such a join that cannot be index accelerated? Is it a cross join that will yield no result after doing all the work?

---

pkg/sql/opt/xform/testdata/rules/select, line 4861 at r2 (raw file):

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

which would mean avoiding the wasteful scan (though I guess this is not a query that will really occur in practice)?

[mgartner]

Let me replay this back: with a constant untyped NULL argument we rewrite the function to tree.DNull in the optimizer when type checking, but when it is a typed NULL we wait until execution time in eval.go to skip the evaluation -- is that accurate?

Exactly.

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner)

[mgartner]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @rytaft and @sumeerbhola)

---

pkg/sql/logictest/testdata/logic_test/inverted_join_geospatial, line 491 at r2 (raw file):

Previously, sumeerbhola wrote…

what plan is generated for such a join that cannot be index accelerated? Is it a cross join that will yield no result after doing all the work?

Correct. The plan looks like this:

query T
EXPLAIN (OPT, VERBOSE) SELECT * FROM t62686 t1 JOIN t62686 t2 ON ST_DFullyWithin(t1.c, t2.c, NULL::FLOAT8)
----
inner-join (cross)
 ├── columns: c:1 c:6
 ├── immutable
 ├── stats: [rows=9801]
 ├── cost: 100012179
 ├── scan t62686 [as=t1]
 │    ├── columns: t1.c:1
 │    ├── stats: [rows=1000, distinct(1)=100, null(1)=10]
 │    ├── cost: 1074.61
 │    ├── prune: (1)
 │    └── unfiltered-cols: (1-5)
 ├── scan t62686 [as=t2]
 │    ├── columns: t2.c:6
 │    ├── stats: [rows=1000, distinct(6)=100, null(6)=10]
 │    ├── cost: 1074.61
 │    ├── prune: (6)
 │    └── unfiltered-cols: (6-10)
 └── filters
      └── st_dfullywithin(t1.c:1, t2.c:6, CAST(NULL AS FLOAT8)) [outer=(1,6), immutable, constraints=(/1: (/NULL - ]; /6: (/NULL - ])]

This could also be normalized to an empty Values expression (no scans or joins) because it should always result in no rows.

---

pkg/sql/opt/xform/testdata/rules/select, line 4861 at r2 (raw file):

Previously, sumeerbhola wrote…

which would mean avoiding the wasteful scan (though I guess this is not a query that will really occur in practice)?

Correct. ST_DFullyWithin(c, ST_GeomFromText('POINT(1 1)'), NULL::FLOAT8) will always evaluate to NULL, so it's a contradiction. There's no need to scan at all.

[sumeerbhola]

Reviewed 1 of 1 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 2 stale) (waiting on @sumeerbhola)

[mgartner]

TFTR!

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)