From 8c60e971b9231cb4daa0e9ff721ca1467bdeeab3 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Mon, 9 Sep 2019 17:54:47 -0700 Subject: [PATCH] exec: minor clean up Previously, when converting a columnar value to a tree.Datum, we would check for nulls separately. Now this is done as part of the conversion which cleans up the code a bit. Release note: None --- pkg/sql/distsqlrun/materializer.go | 11 +---------- pkg/sql/exec/builtin_funcs.go | 8 ++------ pkg/sql/exec/vec_elem_to_datum.go | 9 ++++++++- pkg/sql/row/cfetcher.go | 3 --- pkg/sql/sem/tree/expr.go | 3 ++- 5 files changed, 13 insertions(+), 21 deletions(-) diff --git a/pkg/sql/distsqlrun/materializer.go b/pkg/sql/distsqlrun/materializer.go index 3e2aa85410d0..1677eb1edcca 100644 --- a/pkg/sql/distsqlrun/materializer.go +++ b/pkg/sql/distsqlrun/materializer.go @@ -18,7 +18,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/distsqlpb" "github.com/cockroachdb/cockroach/pkg/sql/exec" "github.com/cockroachdb/cockroach/pkg/sql/exec/execerror" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/sql/types" ) @@ -139,7 +138,7 @@ func (m *materializer) nextAdapter() { // next is the logic of Next() extracted in a separate method to be used by an // adapter to be able to wrap the latter with a catcher. func (m *materializer) next() (sqlbase.EncDatumRow, *distsqlpb.ProducerMetadata) { - for m.State == StateRunning { + if m.State == StateRunning { if m.batch == nil || m.curIdx >= m.batch.Length() { // Get a fresh batch. m.batch = m.input.Next(m.Ctx) @@ -161,14 +160,6 @@ func (m *materializer) next() (sqlbase.EncDatumRow, *distsqlpb.ProducerMetadata) typs := m.OutputTypes() for colIdx := 0; colIdx < len(typs); colIdx++ { col := m.batch.ColVec(colIdx) - // TODO(asubiotto): we shouldn't have to do this check. Figure out who's - // not setting nulls. - if col.MaybeHasNulls() { - if col.Nulls().NullAt(rowIdx) { - m.row[colIdx].Datum = tree.DNull - continue - } - } m.row[colIdx].Datum = exec.PhysicalTypeColElemToDatum(col, rowIdx, m.da, typs[colIdx]) } return m.ProcessRowHelper(m.row), nil diff --git a/pkg/sql/exec/builtin_funcs.go b/pkg/sql/exec/builtin_funcs.go index 7fdc75a7c4dc..6cccb09483ba 100644 --- a/pkg/sql/exec/builtin_funcs.go +++ b/pkg/sql/exec/builtin_funcs.go @@ -63,12 +63,8 @@ func (b *defaultBuiltinFuncOperator) Next(ctx context.Context) coldata.Batch { for j := range b.argumentCols { col := batch.ColVec(b.argumentCols[j]) - if col.MaybeHasNulls() && col.Nulls().NullAt(rowIdx) { - hasNulls = true - b.row[j] = tree.DNull - } else { - b.row[j] = PhysicalTypeColElemToDatum(col, rowIdx, b.da, b.columnTypes[b.argumentCols[j]]) - } + b.row[j] = PhysicalTypeColElemToDatum(col, rowIdx, b.da, b.columnTypes[b.argumentCols[j]]) + hasNulls = hasNulls || b.row[j] == tree.DNull } var ( diff --git a/pkg/sql/exec/vec_elem_to_datum.go b/pkg/sql/exec/vec_elem_to_datum.go index 5a270c8852cb..2fd0960f59d8 100644 --- a/pkg/sql/exec/vec_elem_to_datum.go +++ b/pkg/sql/exec/vec_elem_to_datum.go @@ -23,10 +23,17 @@ import ( "github.com/lib/pq/oid" ) -// PhysicalTypeColElemToDatum converts an element in a colvec to a datum of semtype ct. +// PhysicalTypeColElemToDatum converts an element in a colvec to a datum of +// semtype ct. Note that this function handles nulls as well, so there is no +// need for a separate null check. func PhysicalTypeColElemToDatum( col coldata.Vec, rowIdx uint16, da sqlbase.DatumAlloc, ct types.T, ) tree.Datum { + if col.MaybeHasNulls() { + if col.Nulls().NullAt(rowIdx) { + return tree.DNull + } + } switch ct.Family() { case types.BoolFamily: if col.Bool()[rowIdx] { diff --git a/pkg/sql/row/cfetcher.go b/pkg/sql/row/cfetcher.go index 4e6df34359a1..2df3c27be308 100644 --- a/pkg/sql/row/cfetcher.go +++ b/pkg/sql/row/cfetcher.go @@ -786,9 +786,6 @@ func (rf *CFetcher) pushState(state fetcherState) { // getDatumAt returns the converted datum object at the given (colIdx, rowIdx). // This function is meant for tracing and should not be used in hot paths. func (rf *CFetcher) getDatumAt(colIdx int, rowIdx uint16, typ types.T) tree.Datum { - if rf.machine.colvecs[colIdx].Nulls().NullAt(rowIdx) { - return tree.DNull - } return exec.PhysicalTypeColElemToDatum(rf.machine.colvecs[colIdx], rowIdx, rf.table.da, typ) } diff --git a/pkg/sql/sem/tree/expr.go b/pkg/sql/sem/tree/expr.go index 701f8c4c2222..779502848149 100644 --- a/pkg/sql/sem/tree/expr.go +++ b/pkg/sql/sem/tree/expr.go @@ -1326,7 +1326,8 @@ func (node *FuncExpr) IsDistSQLBlacklist() bool { return node.fnProps != nil && node.fnProps.DistsqlBlacklist } -// CanHandleNulls returns whether or not +// CanHandleNulls returns whether or not the function can handle null +// arguments. func (node *FuncExpr) CanHandleNulls() bool { return node.fnProps != nil && node.fnProps.NullableArgs }