Merge #40615

40615: exec: make sure that AND operator pays attention to whether a value is null r=yuzefovich a=yuzefovich

**exec: make sure that AND operator pays attention to whether a value is null**

Previously, AND operator would simply logically and two boolean vectors.
However, this is incorrect if we have actual null values in those
vectors. The underlying complication is that nulls are stored separately,
so we need to explicitly check for them. Now this is fixed.

**exec: add unit test for AND operator**

This commit adds a unit test for AND operator. It also extends our
testing infrastructure to be able to specify the types schema (this
was needed because nil values are treated as Int64 by default which
is incorrect for tests of AND operator).

Release note: None

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

Makefile

@@ -786,6 +786,7 @@ DOCGEN_TARGETS := bin/.docgen_bnfs bin/.docgen_functions
 
 EXECGEN_TARGETS = \
   pkg/col/coldata/vec.eg.go \
+  pkg/sql/exec/and.eg.go \
   pkg/sql/exec/any_not_null_agg.eg.go \
   pkg/sql/exec/avg_agg.eg.go \
   pkg/sql/exec/cast.eg.go \
@@ -1466,6 +1467,7 @@ $(SETTINGS_DOC_PAGE): $(settings-doc-gen)
 	@$(settings-doc-gen) gen settings-list --format=html > $@
 
 pkg/col/coldata/vec.eg.go: pkg/col/coldata/vec_tmpl.go
+pkg/sql/exec/and.eg.go: pkg/sql/exec/and_tmpl.go
 pkg/sql/exec/any_not_null_agg.eg.go: pkg/sql/exec/any_not_null_agg_tmpl.go
 pkg/sql/exec/avg_agg.eg.go: pkg/sql/exec/avg_agg_tmpl.go
 pkg/sql/exec/cast.eg.go: pkg/sql/exec/cast_tmpl.go

pkg/sql/exec/.gitignore

@@ -1,3 +1,4 @@
+and.eg.go
 any_not_null_agg.eg.go
 avg_agg.eg.go
 cast.eg.go

pkg/sql/exec/aggregator_test.go

@@ -263,7 +263,7 @@ func TestAggregatorOneFunc(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			tupleSource := newOpTestInput(uint16(tc.batchSize), tc.input)
+			tupleSource := newOpTestInput(uint16(tc.batchSize), tc.input, nil /* typs */)
 			a, err := NewOrderedAggregator(
 				tupleSource,
 				tc.colTypes,

pkg/sql/exec/and_test.go

@@ -0,0 +1,94 @@
+// Copyright 2019 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package exec
+
+import (
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/col/coltypes"
+)
+
+func TestAndOp(t *testing.T) {
+	tcs := []struct {
+		tuples   []tuple
+		expected []tuple
+	}{
+		// All variations of pairs separately first.
+		{
+			tuples:   tuples{{false, true}},
+			expected: tuples{{false}},
+		},
+		{
+			tuples:   tuples{{false, nil}},
+			expected: tuples{{false}},
+		},
+		{
+			tuples:   tuples{{false, false}},
+			expected: tuples{{false}},
+		},
+		{
+			tuples:   tuples{{true, true}},
+			expected: tuples{{true}},
+		},
+		{
+			tuples:   tuples{{true, false}},
+			expected: tuples{{false}},
+		},
+		{
+			tuples:   tuples{{true, nil}},
+			expected: tuples{{nil}},
+		},
+		{
+			tuples:   tuples{{nil, true}},
+			expected: tuples{{nil}},
+		},
+		{
+			tuples:   tuples{{nil, false}},
+			expected: tuples{{false}},
+		},
+		{
+			tuples:   tuples{{nil, nil}},
+			expected: tuples{{nil}},
+		},
+		// Now all variations of pairs combined together to make sure that nothing
+		// funky going on with multiple tuples.
+		{
+			tuples: tuples{
+				{false, true}, {false, nil}, {false, false},
+				{true, true}, {true, false}, {true, nil},
+				{nil, true}, {nil, false}, {nil, nil},
+			},
+			expected: tuples{
+				{false}, {false}, {false},
+				{true}, {false}, {nil},
+				{nil}, {false}, {nil},
+			},
+		},
+	}
+
+	for _, tc := range tcs {
+		runTestsWithTyps(
+			t,
+			[]tuples{tc.tuples},
+			[]coltypes.T{coltypes.Bool, coltypes.Bool},
+			tc.expected,
+			orderedVerifier,
+			[]int{2},
+			func(input []Operator) (Operator, error) {
+				return NewAndOp(
+					input[0],
+					0, /* leftIdx */
+					1, /* rightIdx */
+					2, /* outputIdx */
+				), nil
+			})
+	}
+}

pkg/sql/exec/and_tmpl.go

@@ -0,0 +1,152 @@
+// Copyright 2019 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+// {{/*
+// +build execgen_template
+//
+// This file is the execgen template for and.eg.go. It's formatted in a
+// special way, so it's both valid Go and a valid text/template input. This
+// permits editing this file with editor support.
+//
+// */}}
+
+package exec
+
+import (
+	"context"
+
+	"github.com/cockroachdb/cockroach/pkg/col/coldata"
+	"github.com/cockroachdb/cockroach/pkg/col/coltypes"
+)
+
+type andOp struct {
+	OneInputNode
+
+	leftIdx   int
+	rightIdx  int
+	outputIdx int
+}
+
+// NewAndOp returns a new operator that logical-ANDs the boolean columns at
+// leftIdx and rightIdx, returning the result in outputIdx.
+func NewAndOp(input Operator, leftIdx, rightIdx, outputIdx int) Operator {
+	return &andOp{
+		OneInputNode: NewOneInputNode(input),
+		leftIdx:      leftIdx,
+		rightIdx:     rightIdx,
+		outputIdx:    outputIdx,
+	}
+}
+
+func (a *andOp) Init() {
+	a.input.Init()
+}
+
+// {{/*
+// This code snippet sets the result of AND'ing two boolean vectors while
+// paying attention to null values.
+func _SET_VALUES(_L_HAS_NULLS bool, _R_HAS_NULLS bool) { // */}}
+	// {{ define "setValues" -}}
+	if sel := batch.Selection(); sel != nil {
+		for _, i := range sel[:n] {
+			_SET_SINGLE_VALUE(true, _L_HAS_NULLS, _R_HAS_NULLS)
+		}
+	} else {
+		_ = rightColVals[n-1]
+		_ = outputColVals[n-1]
+		for i := range leftColVals[:n] {
+			_SET_SINGLE_VALUE(false, _L_HAS_NULLS, _R_HAS_NULLS)
+		}
+	}
+	// {{ end }}
+	// {{/*
+}
+
+// */}}
+
+// {{/*
+// This code snippet sets the result of AND'ing two boolean values which can be
+// null.
+// The rules for AND'ing two booleans are:
+// 1. if at least one of the values is FALSE, then the result is also FALSE
+// 2. if both values are TRUE, then the result is also TRUE
+// 3. in all other cases (one is TRUE and the other is NULL or both are NULL),
+//    the result is NULL.
+func _SET_SINGLE_VALUE(_USES_SEL bool, _L_HAS_NULLS bool, _R_HAS_NULLS bool) { // */}}
+	// {{ define "setSingleValue" -}}
+	// {{ if _USES_SEL }}
+	idx := i
+	// {{ else }}
+	idx := uint16(i)
+	// {{ end }}
+	// {{ if _L_HAS_NULLS }}
+	isLeftNull := leftNulls.NullAt(idx)
+	// {{ else }}
+	isLeftNull := false
+	// {{ end }}
+	// {{ if _R_HAS_NULLS }}
+	isRightNull := rightNulls.NullAt(idx)
+	// {{ else }}
+	isRightNull := false
+	// {{ end }}
+	leftVal := leftColVals[idx]
+	rightVal := rightColVals[idx]
+	if (!leftVal && !isLeftNull) || (!rightVal && !isRightNull) {
+		// Rule 1: at least one boolean is FALSE.
+		outputColVals[idx] = false
+	} else if (leftVal && !isLeftNull) && (rightVal && !isRightNull) {
+		// Rule 2: both booleans are TRUE.
+		outputColVals[idx] = true
+	} else {
+		// Rule 3.
+		outputNulls.SetNull(idx)
+	}
+	// {{ end }}
+	// {{/*
+}
+
+// */}}
+
+func (a *andOp) Next(ctx context.Context) coldata.Batch {
+	batch := a.input.Next(ctx)
+	if a.outputIdx == batch.Width() {
+		batch.AppendCol(coltypes.Bool)
+	}
+	n := batch.Length()
+	if n == 0 {
+		return batch
+	}
+	leftCol := batch.ColVec(a.leftIdx)
+	rightCol := batch.ColVec(a.rightIdx)
+	outputCol := batch.ColVec(a.outputIdx)
+
+	leftColVals := leftCol.Bool()
+	rightColVals := rightCol.Bool()
+	outputColVals := outputCol.Bool()
+	outputNulls := outputCol.Nulls()
+	if leftCol.MaybeHasNulls() {
+		leftNulls := leftCol.Nulls()
+		if rightCol.MaybeHasNulls() {
+			rightNulls := rightCol.Nulls()
+			_SET_VALUES(true, true)
+		} else {
+			_SET_VALUES(true, false)
+		}
+	} else {
+		if rightCol.MaybeHasNulls() {
+			rightNulls := rightCol.Nulls()
+			_SET_VALUES(false, true)
+		} else {
+			_SET_VALUES(false, false)
+		}
+	}
+
+	return batch
+}

pkg/sql/exec/execgen/cmd/execgen/and_gen.go

@@ -0,0 +1,46 @@
+// Copyright 2019 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package main
+
+import (
+	"io"
+	"io/ioutil"
+	"strings"
+	"text/template"
+)
+
+func genAndOp(wr io.Writer) error {
+	t, err := ioutil.ReadFile("pkg/sql/exec/and_tmpl.go")
+	if err != nil {
+		return err
+	}
+
+	s := string(t)
+	s = strings.Replace(s, "_L_HAS_NULLS", "$.lHasNulls", -1)
+	s = strings.Replace(s, "_R_HAS_NULLS", "$.rHasNulls", -1)
+	s = strings.Replace(s, "_USES_SEL", "$.usesSel", -1)
+
+	setValues := makeFunctionRegex("_SET_VALUES", 2)
+	s = setValues.ReplaceAllString(s, `{{template "setValues" buildDict "Global" $ "lHasNulls" $1 "rHasNulls" $2}}`)
+	setSingleValue := makeFunctionRegex("_SET_SINGLE_VALUE", 3)
+	s = setSingleValue.ReplaceAllString(s, `{{template "setSingleValue" buildDict "Global" $ "usesSel" $1 "lHasNulls" $2 "rHasNulls" $3}}`)
+
+	tmpl, err := template.New("and").Funcs(template.FuncMap{"buildDict": buildDict}).Parse(s)
+	if err != nil {
+		return err
+	}
+
+	return tmpl.Execute(wr, nil /* data */)
+}
+
+func init() {
+	registerGenerator(genAndOp, "and.eg.go")
+}