Merge #151093 #151137

151093: colexec: fix column type when ConstNullOp is used r=yuzefovich a=yuzefovich

In the vectorized engine, each projecting operator is responsible for appending its output vector to the batch. Previously, we had a bug in an edge case where we have a "typed" NULL constant vector. Namely, we would create the VectorTypeEnforcer helper with `types.Unknown`, yet we would include the actual type of the NULL column (by appending the actual output type to `typs` in `execplan.go`), which would lead to problems down the line. The fix is simple - use the actual type for the vector even if we're going to mark all values in it as NULL.

I struggled to come up with a scenario where we would hit this without disabling some folding optimizations, so the regression test added here disables the "always null" short-circuiting logic during type checking in test builds, via a testing knob.

Fixes: #151020.

Release note (bug fix): Previously, CockroachDB could encounter an internal error "trying to add a column of UNKNOWN type at ..." in rare cases when handling CASE or OR operations. The bug has been present since 20.2 version and is now fixed.

151137: psql: delete unnecessary `toolchain` line r=rail a=rickystewart

It's not necessary and some automation auto-deletes it.

Epic: DEVINF-1477
Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>

Author: 3 people

pkg/acceptance/compose/gss/psql/go.mod

@@ -2,8 +2,6 @@ module github.com/cockroachdb/cockroach/pkg/acceptance/compose/gss/psql
 
 go 1.24.5
 
-toolchain go1.24.5
-
 require (
 	github.com/cockroachdb/errors v1.9.0
 	github.com/lib/pq v1.10.6

pkg/sql/colexec/colbuilder/execplan.go

@@ -2386,7 +2386,7 @@ func planProjectionOperators(
 		if datumType.Family() == types.UnknownFamily {
 			// We handle Unknown type by planning a special constant null
 			// operator.
-			return colexecbase.NewConstNullOp(allocator, input, resultIdx), nil
+			return colexecbase.NewConstNullOp(allocator, datumType, input, resultIdx), nil
 		}
 		constVal := colconv.GetDatumToPhysicalFn(datumType)(datum)
 		return colexecbase.NewConstOp(allocator, input, datumType, constVal, resultIdx)
@@ -2866,7 +2866,7 @@ func planProjectionExpr(
 			if !calledOnNullInput && right == tree.DNull {
 				// If the right is NULL and the operator is not called on NULL,
 				// simply project NULL.
-				op = colexecbase.NewConstNullOp(allocator, input, resultIdx)
+				op = colexecbase.NewConstNullOp(allocator, outputType, input, resultIdx)
 			} else if isCmpProjOp {
 				// Use optimized operators for special cases.
 				switch cmpProjOp.Symbol {

pkg/sql/colexec/colexecbase/const.eg.go

@@ -127,12 +127,12 @@ func (c const_TYPEOp) Next() coldata.Batch {
 // {{end}}
 // {{end}}
 
-// NewConstNullOp creates a new operator that produces a constant (untyped) NULL
-// value at index outputIdx.
+// NewConstNullOp creates a new operator that produces a constant NULL value at
+// index outputIdx. The column will be typed according to the passed in 't'.
 func NewConstNullOp(
-	allocator *colmem.Allocator, input colexecop.Operator, outputIdx int,
+	allocator *colmem.Allocator, t *types.T, input colexecop.Operator, outputIdx int,
 ) colexecop.Operator {
-	input = colexecutils.NewVectorTypeEnforcer(allocator, input, types.Unknown, outputIdx)
+	input = colexecutils.NewVectorTypeEnforcer(allocator, input, t, outputIdx)
 	return &constNullOp{
 		OneInputHelper: colexecop.MakeOneInputHelper(input),
 		outputIdx:      outputIdx,

pkg/sql/colexec/colexecbase/const_tmpl.go

@@ -34,6 +34,7 @@ go_test(
         "//pkg/util/log",
         "//pkg/util/randutil",
         "@com_github_cockroachdb_datadriven//:datadriven",
+        "@com_github_cockroachdb_errors//:errors",
         "@com_github_stretchr_testify//require",
     ],
 )

pkg/sql/sem/eval/eval_test/BUILD.bazel

@@ -34,6 +34,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	"github.com/cockroachdb/datadriven"
+	"github.com/cockroachdb/errors"
 	"github.com/stretchr/testify/require"
 )
 
@@ -156,6 +157,8 @@ func TestEval(t *testing.T) {
 				return strings.TrimSpace(d.Expected)
 			}
 			semaCtx := tree.MakeSemaContext(nil /* resolver */)
+			// In 50% cases, disable "always null" short-circuiting.
+			semaCtx.TestingKnobs.DisallowAlwaysNullShortCut = rng.Intn(2) == 1
 			typedExpr, err := expr.TypeCheck(ctx, &semaCtx, types.AnyElement)
 			if err != nil {
 				// Skip this test as it's testing an expected error which would be
@@ -221,6 +224,9 @@ func TestEval(t *testing.T) {
 			row, meta := mat.Next()
 			if meta != nil {
 				if meta.Err != nil {
+					if errors.IsAssertionFailure(meta.Err) {
+						t.Fatalf("%+v", meta.Err)
+					}
 					return fmt.Sprint(meta.Err)
 				}
 				t.Fatalf("unexpected metadata: %+v", meta)

pkg/sql/sem/eval/eval_test/eval_test.go

@@ -114,3 +114,10 @@ eval
 true OR (3 = 1)
 ----
 true
+
+# Regression test for incorrect typing of NULL column in the vectorized engine
+# (#151020).
+eval
+('foo' LIKE NULL) OR true
+----
+true

pkg/sql/sem/eval/testdata/eval/boolean

@@ -16,6 +16,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treecmp"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/volatility"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
+	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
 	"github.com/cockroachdb/cockroach/pkg/util/collatedstring"
 	"github.com/cockroachdb/cockroach/pkg/util/duration"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
@@ -78,6 +79,13 @@ type SemaContext struct {
 	// UsePre_25_2VariadicBuiltins is set to true when we should use the pre-25.2
 	// variadic builtins behavior.
 	UsePre_25_2VariadicBuiltins bool
+
+	// TestingKnobs only has effect under buildutil.CrdbTestBuild.
+	TestingKnobs struct {
+		// DisallowAlwaysNullShortCut, if set, disables short-circuiting logic
+		// for "always NULL" case during type checking.
+		DisallowAlwaysNullShortCut bool
+	}
 }
 
 // SemaProperties is a holder for required and derived properties
@@ -2370,9 +2378,12 @@ func typeCheckComparisonOpWithSubOperator(
 		rightTyped = array
 		cmpTypeRight = retType
 
-		// Return early without looking up a CmpOp if the comparison type is types.Null.
-		if leftTyped.ResolvedType().Family() == types.UnknownFamily || retType.Family() == types.UnknownFamily {
-			return leftTyped, rightTyped, nil, true /* alwaysNull */, nil
+		// Return early without looking up a CmpOp if the comparison type is types.Null
+		// (unless the short-cut is disabled in tests).
+		if !buildutil.CrdbTestBuild || semaCtx == nil || !semaCtx.TestingKnobs.DisallowAlwaysNullShortCut {
+			if leftTyped.ResolvedType().Family() == types.UnknownFamily || retType.Family() == types.UnknownFamily {
+				return leftTyped, rightTyped, nil, true /* alwaysNull */, nil
+			}
 		}
 	} else {
 		// If the right expression is not an array constructor, we type the left
@@ -2403,8 +2414,10 @@ func typeCheckComparisonOpWithSubOperator(
 		}
 
 		rightReturn := rightTyped.ResolvedType()
-		if rightReturn.Family() == types.UnknownFamily {
-			return leftTyped, rightTyped, nil, true /* alwaysNull */, nil
+		if !buildutil.CrdbTestBuild || semaCtx == nil || !semaCtx.TestingKnobs.DisallowAlwaysNullShortCut {
+			if rightReturn.Family() == types.UnknownFamily {
+				return leftTyped, rightTyped, nil, true /* alwaysNull */, nil
+			}
 		}
 
 		switch rightReturn.Family() {
@@ -2739,8 +2752,10 @@ func typeCheckComparisonOp(
 					break
 				}
 			}
-			if noneAcceptNull {
-				return leftExpr, rightExpr, nil, true /* alwaysNull */, nil
+			if !buildutil.CrdbTestBuild || semaCtx == nil || !semaCtx.TestingKnobs.DisallowAlwaysNullShortCut {
+				if noneAcceptNull {
+					return leftExpr, rightExpr, nil, true /* alwaysNull */, nil
+				}
 			}
 		}
 	}