colexec: fix IS NOT NULL filter for tuples with NULLs

This commit fixes `isTupleNullSelOp` (which handles IS NOT NULL filter
expression). Previously, we incorrectly short-circuited the filter
evaluation whenever the tuple is non-NULL - all such tuples were passed
by the filter even though only tuples that don't have any NULL elements
should pass. This is now fixed by bringing the logic inline with how
row-by-row engine does it as well as the `isTupleNullProjOp` does it.

This commit additionally extends an existing vectorized eval test (which
currently runs all projections) to also run filtering (i.e. "selection"
operators) on expressions of the boolean type. This would have caught
the bug.

Release note (bug fix): Previously, CockroachDB could incorrectly
evaluate `IS NOT NULL` filter if it was applied to non-NULL tuples that
had NULL elements (like `(1, NULL)` or `(NULL, NULL)`). The bug is
present since 20.2 version and is now fixed.

pkg/sql/colexec/is_null_ops_tmpl.go

@@ -244,8 +244,10 @@ func _MAYBE_SELECT(
 	// {{define "maybeSelect" -}}
 
 	// {{if .IsTuple}}
-	selectTuple := nulls.NullAt(i) != o.negate
-	if !selectTuple {
+	var selectTuple bool
+	if nulls.NullAt(i) {
+		selectTuple = !o.negate
+	} else {
 		selectTuple = isTupleNull(datums.Get(i).(tree.Datum), o.negate)
 	}
 	if selectTuple {

pkg/sql/logictest/testdata/logic_test/tuple

@@ -1307,3 +1307,28 @@ RESET testing_optimizer_random_seed;
 RESET testing_optimizer_disable_rule_probability;
 RESET vectorize;
 RESET distsql_workmem;
+
+# Regression test for incorrect handling of IS NOT NULL filter in the vectorized
+# engine (#126769).
+query T
+SELECT * FROM (SELECT (NULL, NULL) AS a) AS b WHERE a IS NOT NULL;
+----
+
+query T
+SELECT * FROM (SELECT (1, NULL) AS a) AS b WHERE a IS NOT NULL;
+----
+
+query T
+SELECT * FROM (SELECT (1, 2) AS a) AS b WHERE a IS NOT NULL;
+----
+(1,2)
+
+query B
+SELECT ROW() IS NULL;
+----
+true
+
+query B
+SELECT ROW() IS NOT NULL;
+----
+true

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

@@ -23,7 +23,6 @@ go_test(
         "//pkg/sql/execinfra",
         "//pkg/sql/execinfrapb",
         "//pkg/sql/parser",
-        "//pkg/sql/rowenc",
         "//pkg/sql/rowexec",
         "//pkg/sql/sem/builtins",
         "//pkg/sql/sem/eval",

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

@@ -29,7 +29,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfra"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
-	"github.com/cockroachdb/cockroach/pkg/sql/rowenc"
 	"github.com/cockroachdb/cockroach/pkg/sql/rowexec"
 	_ "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
@@ -38,6 +37,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	"github.com/cockroachdb/datadriven"
 	"github.com/stretchr/testify/require"
 )
@@ -141,6 +141,7 @@ func TestEval(t *testing.T) {
 	})
 
 	t.Run("vectorized", func(t *testing.T) {
+		rng, _ := randutil.NewTestRand()
 		var monitorRegistry colexecargs.MonitorRegistry
 		defer monitorRegistry.Close(ctx)
 		walk(t, func(t *testing.T, d *datadriven.TestData) string {
@@ -187,19 +188,31 @@ func TestEval(t *testing.T) {
 							if batchesReturned > 0 {
 								return coldata.ZeroBatch
 							}
-							// It doesn't matter what types we create the input batch with.
+							// It doesn't matter what types we create the input
+							// batch with.
 							batch := coldata.NewMemBatch([]*types.T{}, coldata.StandardColumnFactory)
 							batch.SetLength(1)
 							batchesReturned++
 							return batch
 						}},
 				}},
 				StreamingMemAccount: &acc,
-				// Unsupported post processing specs are wrapped and run through the
-				// row execution engine.
+				// Unsupported post-processing specs are wrapped and run through
+				// the row execution engine.
 				ProcessorConstructor: rowexec.NewProcessor,
 				MonitorRegistry:      &monitorRegistry,
 			}
+			// If the expression is of the boolean type, in 50% cases we'll
+			// additionally run it as a filter (i.e. as a "selection" operator
+			// as opposed to a "projection").
+			var doFilter bool
+			if typedExpr.ResolvedType().Family() == types.BoolFamily && rng.Float64() < 0.5 {
+				doFilter = true
+				args.Spec.Core.Noop = nil
+				args.Spec.Core.Filterer = &execinfrapb.FiltererSpec{
+					Filter: execinfrapb.Expression{LocalExpr: typedExpr},
+				}
+			}
 			result, err := colbuilder.NewColOperator(ctx, flowCtx, args)
 			require.NoError(t, err)
 
@@ -211,19 +224,31 @@ func TestEval(t *testing.T) {
 				[]*types.T{typedExpr.ResolvedType()},
 			)
 
-			var (
-				row  rowenc.EncDatumRow
-				meta *execinfrapb.ProducerMetadata
-			)
 			mat.Start(ctx)
-			row, meta = mat.Next()
+			row, meta := mat.Next()
 			if meta != nil {
 				if meta.Err != nil {
 					return fmt.Sprint(meta.Err)
 				}
 				t.Fatalf("unexpected metadata: %+v", meta)
 			}
-			if row == nil {
+			if doFilter {
+				switch strings.ToLower(strings.TrimSpace(d.Expected)) {
+				case "true":
+					if row == nil {
+						t.Fatalf("the row should not be filtered out: %s", d.Input)
+					}
+				case "false", "null":
+					if row != nil {
+						t.Fatalf("the row should be filtered out: %s", d.Input)
+					}
+					// The row is filtered out, so we just return the expected
+					// output here.
+					return strings.TrimSpace(d.Expected)
+				default:
+					t.Fatalf("unexpected bool value: %s", d.Expected)
+				}
+			} else if row == nil {
 				t.Fatal("unexpected end of input")
 			}
 			return row[0].Datum.String()