From 9ba2fd738834e9047bcee5346457d9f61a23a4ad Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Sat, 19 May 2018 16:40:18 -0400 Subject: [PATCH] sql: fix null normalization The normalization rules are happy to convert `NULL::TEXT` to `NULL`. While both expressions evaluate to `DNull`, the `ResolvedType()` is different. It seems unsound for normalization to change the type. This issue is shown by trying to run a query containing `ARRAY_AGG(NULL::TEXT)` through distsql planning: by the time the distsql planner looks at it, the `NULL::TEXT` is just `DNull` (with the `Unknown` type) and the distsql planner cannot find the builtin. This change fixes the normalization rules by retaining the cast in this case. In general, any expression that statically evaluates to NULL gets a cast to the original expression type. The same is done in the opt execbuilder. Fixes #25724. Release note (bug fix): Fixed query errors in some cases involving a NULL constant that is cast to a specific type. --- .../logictest/testdata/logic_test/aggregate | 7 ++++++ pkg/sql/logictest/testdata/logic_test/join | 4 ++-- .../opt/exec/execbuilder/scalar_builder.go | 2 +- .../opt/exec/execbuilder/testdata/aggregate | 4 +--- pkg/sql/sem/tree/normalize.go | 22 ++++++++++++++----- pkg/sql/sem/tree/normalize_test.go | 6 ++--- 6 files changed, 31 insertions(+), 14 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/aggregate b/pkg/sql/logictest/testdata/logic_test/aggregate index 59ff0708f2d7..7c3128f317f2 100644 --- a/pkg/sql/logictest/testdata/logic_test/aggregate +++ b/pkg/sql/logictest/testdata/logic_test/aggregate @@ -100,6 +100,13 @@ SELECT ARRAY_AGG(NULL::TEXT) ---- {NULL} +# Regression test for #25724 (problem with typed NULLs and distsql planning). +# The previous query doesn't run under distsql. +query T +SELECT ARRAY_AGG(NULL::TEXT) FROM (VALUES (1)) AS t(x) +---- +{NULL} + # Check that COALESCE using aggregate results over an empty table # work properly. query I diff --git a/pkg/sql/logictest/testdata/logic_test/join b/pkg/sql/logictest/testdata/logic_test/join index 754e466f612b..90253d2b3703 100644 --- a/pkg/sql/logictest/testdata/logic_test/join +++ b/pkg/sql/logictest/testdata/logic_test/join @@ -790,11 +790,11 @@ SELECT "Level", "Type", "Field", "Description" FROM [EXPLAIN (VERBOSE) SELECT 0 sort · · 0 · order +pktable_schem,+pktable_name,+fk_name,+key_seq 1 render · · -1 · render 0 NULL +1 · render 0 CAST(NULL AS STRING) 1 · render 1 pkn.nspname 1 · render 2 pkc.relname 1 · render 3 pka.attname -1 · render 4 NULL +1 · render 4 CAST(NULL AS STRING) 1 · render 5 fkn.nspname 1 · render 6 fkc.relname 1 · render 7 fka.attname diff --git a/pkg/sql/opt/exec/execbuilder/scalar_builder.go b/pkg/sql/opt/exec/execbuilder/scalar_builder.go index feefd34772c6..e38f33b45d47 100644 --- a/pkg/sql/opt/exec/execbuilder/scalar_builder.go +++ b/pkg/sql/opt/exec/execbuilder/scalar_builder.go @@ -90,7 +90,7 @@ func (b *Builder) buildTypedExpr(ctx *buildScalarCtx, ev memo.ExprView) (tree.Ty } func (b *Builder) buildNull(ctx *buildScalarCtx, ev memo.ExprView) (tree.TypedExpr, error) { - return tree.DNull, nil + return tree.ReType(tree.DNull, ev.Logical().Scalar.Type) } func (b *Builder) buildVariable(ctx *buildScalarCtx, ev memo.ExprView) (tree.TypedExpr, error) { diff --git a/pkg/sql/opt/exec/execbuilder/testdata/aggregate b/pkg/sql/opt/exec/execbuilder/testdata/aggregate index 71a13f6facb3..0e43cae7308d 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/aggregate +++ b/pkg/sql/opt/exec/execbuilder/testdata/aggregate @@ -204,9 +204,7 @@ SELECT JSONB_AGG(NULL) jsonb_agg:jsonb '[null]' -# With an explicit cast, this works as expected. -# TODO(radu): investigate distsql issue (relate to the input type) -exec nodist +exec SELECT ARRAY_AGG(NULL::TEXT) ---- array_agg:string[] diff --git a/pkg/sql/sem/tree/normalize.go b/pkg/sql/sem/tree/normalize.go index f306daf28376..a0eefa945e18 100644 --- a/pkg/sql/sem/tree/normalize.go +++ b/pkg/sql/sem/tree/normalize.go @@ -27,9 +27,6 @@ type normalizableExpr interface { } func (expr *CastExpr) normalize(v *NormalizeVisitor) TypedExpr { - if expr.Expr == DNull { - return DNull - } return expr } @@ -727,12 +724,27 @@ func (v *NormalizeVisitor) VisitPost(expr Expr) Expr { if _, ok := expr.(*Placeholder); ok { return expr } - newExpr, err := expr.(TypedExpr).Eval(v.ctx) + value, err := expr.(TypedExpr).Eval(v.ctx) if err != nil { + // Ignore any errors here (e.g. division by zero), so they can happen + // during execution where they are correctly handled. Note that in some + // cases we might not even get an error (if this particular expression + // does not get evaluated when the query runs, e.g. it's inside a CASE). return expr } - expr = newExpr + if value == DNull { + // We don't want to return an expression that has a different type; cast + // the NULL if necessary. + var newExpr TypedExpr + newExpr, v.err = ReType(DNull, expr.(TypedExpr).ResolvedType()) + if v.err != nil { + return expr + } + return newExpr + } + return value } + return expr } diff --git a/pkg/sql/sem/tree/normalize_test.go b/pkg/sql/sem/tree/normalize_test.go index 9b9afc6af156..8fe3e67d9b12 100644 --- a/pkg/sql/sem/tree/normalize_test.go +++ b/pkg/sql/sem/tree/normalize_test.go @@ -41,7 +41,7 @@ func TestNormalizeExpr(t *testing.T) { }{ {`(a)`, `a`}, {`((((a))))`, `a`}, - {`CAST(NULL AS INTEGER)`, `NULL`}, + {`CAST(NULL AS INTEGER)`, `CAST(NULL AS INT)`}, {`+a`, `a`}, {`-(-a)`, `a`}, {`-+-a`, `a`}, @@ -100,7 +100,7 @@ func TestNormalizeExpr(t *testing.T) { {`a IN (NULL)`, `NULL`}, {`a IN (NULL, NULL)`, `NULL`}, {`1 IN (1, NULL)`, `true`}, - {`1 IN (2, NULL)`, `NULL`}, + {`1 IN (2, NULL)`, `CAST(NULL AS BOOL)`}, {`1 = ANY ARRAY[3, 2, 1]`, `true`}, {`1 < SOME ARRAY[3, 2, 1]`, `true`}, {`1 > SOME (ARRAY[3, 2, 1])`, `false`}, @@ -156,7 +156,7 @@ func TestNormalizeExpr(t *testing.T) { {`IF((true OR a < 0), (0 + a)::decimal, 2 / (1 - 1))`, `a::DECIMAL`}, {`COALESCE(NULL, (NULL < 3), a = 2 - 1, d)`, `COALESCE(a = 1, d)`}, {`COALESCE(NULL, a)`, `a`}, - {`NOT NULL`, `NULL`}, + {`NOT NULL`, `CAST(NULL AS BOOL)`}, {`NOT d`, `NOT d`}, {`NOT NOT d`, `d`}, {`NOT NOT NOT d`, `NOT d`},