Skip to content

Commit

Permalink
sql: fix null normalization
Browse files Browse the repository at this point in the history
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 cockroachdb#25724.

Release note (bug fix): Fixed query errors in some cases involving a
NULL constant that is cast to a specific type.
  • Loading branch information
RaduBerinde committed May 20, 2018
1 parent 9deff8d commit 20c77df
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 11 deletions.
7 changes: 7 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/aggregate
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/logictest/testdata/logic_test/join
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/opt/exec/execbuilder/scalar_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 1 addition & 3 deletions pkg/sql/opt/exec/execbuilder/testdata/aggregate
Original file line number Diff line number Diff line change
Expand Up @@ -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[]
Expand Down
22 changes: 17 additions & 5 deletions pkg/sql/sem/tree/normalize.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ type normalizableExpr interface {
}

func (expr *CastExpr) normalize(v *NormalizeVisitor) TypedExpr {
if expr.Expr == DNull {
return DNull
}
return expr
}

Expand Down Expand Up @@ -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
}

Expand Down

0 comments on commit 20c77df

Please sign in to comment.