Merge #42968

42968: colexec: check for unsupported output type of a builtin r=yuzefovich a=yuzefovich Previously, we would always create a defaultBuiltinOperator with a converted physical output type from a builtin function. This could be coltypes.Unhandled, and only during runtime we would realize that we do not support the output type of the builtin. Now this is fixed. Fixes: #42871. Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]>
cockroachdb · Dec 5, 2019 · a7d225e · a7d225e
2 parents 3bbe0a8 + 3dcdd48
commit a7d225e
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 8 deletions.
diff --git a/pkg/sql/colexec/builtin_funcs.go b/pkg/sql/colexec/builtin_funcs.go
@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
+	"github.com/cockroachdb/errors"
 )
 
 type defaultBuiltinFuncOperator struct {
@@ -196,7 +197,7 @@ func NewBuiltinFunctionOperator(
 	argumentCols []int,
 	outputIdx int,
 	input Operator,
-) Operator {
+) (Operator, error) {
 
 	switch funcExpr.ResolvedOverload().SpecializedVecBuiltin {
 	case tree.SubstringStringIntInt:
@@ -205,9 +206,16 @@ func NewBuiltinFunctionOperator(
 			allocator:    allocator,
 			argumentCols: argumentCols,
 			outputIdx:    outputIdx,
-		}
+		}, nil
 	default:
 		outputType := funcExpr.ResolvedType()
+		outputPhysType := typeconv.FromColumnType(outputType)
+		if outputPhysType == coltypes.Unhandled {
+			return nil, errors.Errorf(
+				"unsupported output type %q of %s",
+				outputType.String(), funcExpr.String(),
+			)
+		}
 		return &defaultBuiltinFuncOperator{
 			OneInputNode:   NewOneInputNode(input),
 			allocator:      allocator,
@@ -216,10 +224,10 @@ func NewBuiltinFunctionOperator(
 			outputIdx:      outputIdx,
 			columnTypes:    columnTypes,
 			outputType:     outputType,
-			outputPhysType: typeconv.FromColumnType(outputType),
+			outputPhysType: outputPhysType,
 			converter:      typeconv.GetDatumToPhysicalFn(outputType),
 			row:            make(tree.Datums, len(argumentCols)),
 			argumentCols:   argumentCols,
-		}
+		}, nil
 	}
 }
diff --git a/pkg/sql/colexec/builtin_funcs_test.go b/pkg/sql/colexec/builtin_funcs_test.go
@@ -25,6 +25,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/stretchr/testify/require"
 )
 
 // Mock typing context for the typechecker.
@@ -96,7 +97,7 @@ func TestBasicBuiltinFunctions(t *testing.T) {
 						t.Fatal(err)
 					}
 
-					return NewBuiltinFunctionOperator(testAllocator, tctx, typedExpr.(*tree.FuncExpr), tc.outputTypes, tc.inputCols, 1, input[0]), nil
+					return NewBuiltinFunctionOperator(testAllocator, tctx, typedExpr.(*tree.FuncExpr), tc.outputTypes, tc.inputCols, 1, input[0])
 				})
 		})
 	}
@@ -147,7 +148,8 @@ func benchmarkBuiltinFunctions(b *testing.B, useSelectionVector bool, hasNulls b
 	if err != nil {
 		b.Fatal(err)
 	}
-	op := NewBuiltinFunctionOperator(testAllocator, tctx, typedExpr.(*tree.FuncExpr), []types.T{*types.Int}, []int{0}, 1, source)
+	op, err := NewBuiltinFunctionOperator(testAllocator, tctx, typedExpr.(*tree.FuncExpr), []types.T{*types.Int}, []int{0}, 1, source)
+	require.NoError(b, err)
 
 	b.SetBytes(int64(8 * coldata.BatchSize()))
 	b.ResetTimer()

diff --git a/pkg/sql/colexec/execplan.go b/pkg/sql/colexec/execplan.go
@@ -1274,8 +1274,8 @@ func planProjectionOperators(
 		funcOutputType := t.ResolvedType()
 		resultIdx = len(ct)
 		ct = append(ct, *funcOutputType)
-		op = NewBuiltinFunctionOperator(NewAllocator(ctx, acc), evalCtx, t, ct, inputCols, resultIdx, op)
-		return op, resultIdx, ct, internalMemUsed, nil
+		op, err = NewBuiltinFunctionOperator(NewAllocator(ctx, acc), evalCtx, t, ct, inputCols, resultIdx, op)
+		return op, resultIdx, ct, internalMemUsed, err
 	case tree.Datum:
 		datumType := t.ResolvedType()
 		ct = columnTypes

diff --git a/pkg/sql/logictest/testdata/logic_test/vectorize_types b/pkg/sql/logictest/testdata/logic_test/vectorize_types
@@ -83,3 +83,8 @@ SELECT _id2, _bool, _bool2 FROM skip_unneeded_cols
 
 statement ok
 RESET vectorize
+
+# This query uses a builtin that returns currently unsupported type
+# (TimestampTZ). We're only interested in not getting an error. See #42871.
+statement ok
+SELECT experimental_strptime(_string, _string) IS NULL FROM all_types