Skip to content

Commit

Permalink
exec: make sure that AND operator pays attention to whether a value i…
Browse files Browse the repository at this point in the history
…s 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.

Release note: None
  • Loading branch information
yuzefovich committed Sep 11, 2019
1 parent 832b2bd commit 10193ad
Show file tree
Hide file tree
Showing 6 changed files with 217 additions and 68 deletions.
2 changes: 2 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/exec/.gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
and.eg.go
any_not_null_agg.eg.go
avg_agg.eg.go
cast.eg.go
Expand Down
68 changes: 0 additions & 68 deletions pkg/sql/exec/and.go

This file was deleted.

152 changes: 152 additions & 0 deletions pkg/sql/exec/and_tmpl.go
Original file line number Diff line number Diff line change
@@ -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
}
46 changes: 46 additions & 0 deletions pkg/sql/exec/execgen/cmd/execgen/and_gen.go
Original file line number Diff line number Diff line change
@@ -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")
}
16 changes: 16 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/vectorize
Original file line number Diff line number Diff line change
Expand Up @@ -846,3 +846,19 @@ INSERT INTO tnull VALUES(NULL, 238)
query I rowsort
SELECT a FROM tnull WHERE (a<=b OR a>=b)
----

# Test that AND'ing a true value with another true value while one of them is
# actually NULL returns NULL.
statement ok
CREATE TABLE t1(a INTEGER, b INTEGER, c INTEGER)

statement ok
INSERT INTO t1 VALUES(NULL,2,1)

# We need both parenthesis in WHERE clause so that the AND operation under test
# is not optimized out.
query I
SELECT CASE WHEN a <= b THEN 1 ELSE 2 END
FROM t1
WHERE (a > b - 2 AND a < b + 2) OR (c > a AND c < b)
----

0 comments on commit 10193ad

Please sign in to comment.