Merge cockroachdb#126901

126901: colexec: fix IS NOT NULL filter for tuples with NULLs r=yuzefovich a=yuzefovich

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.

Fixes: cockroachdb#126769.

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.

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

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()