From af72a3e72b1868b9c48286e86df04a70efa7d158 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Wed, 31 Mar 2021 12:00:15 -0700 Subject: [PATCH] opt: fix geospatial function NULL argument panics 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. --- .../logic_test/inverted_join_geospatial | 12 +++++ pkg/sql/opt/invertedidx/geo.go | 20 +++++--- pkg/sql/opt/xform/testdata/rules/select | 47 +++++++++++++++++++ 3 files changed, 73 insertions(+), 6 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/inverted_join_geospatial b/pkg/sql/logictest/testdata/logic_test/inverted_join_geospatial index 066328f870d4..8927ba46cb17 100644 --- a/pkg/sql/logictest/testdata/logic_test/inverted_join_geospatial +++ b/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) diff --git a/pkg/sql/opt/invertedidx/geo.go b/pkg/sql/opt/invertedidx/geo.go index 3912d27f5380..7c27c566e6b4 100644 --- a/pkg/sql/opt/invertedidx/geo.go +++ b/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))) diff --git a/pkg/sql/opt/xform/testdata/rules/select b/pkg/sql/opt/xform/testdata/rules/select index d649476e8411..30cd50adbd8a 100644 --- a/pkg/sql/opt/xform/testdata/rules/select +++ b/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 # --------------------------------------------------