From 99711d964ff72972a1dc79a5be91f0ed1de92509 Mon Sep 17 00:00:00 2001
From: Jordan Lewis <jordanthelewis@gmail.com>
Date: Tue, 10 Sep 2019 00:23:45 -0400
Subject: [PATCH] coldata: bugfix to coldata.Copy with NULL and SelOnDest

Previously, Copy always wiped all of the destination NULLs regardless of
SelOnDest, which if set indiciates that only the selection vector tuples
will be written to on the destinatiion side. Now, when SelOnDest is
true, we don't erroneously overwrite non-selected nulls.

Release note: None
---
 pkg/col/coldata/vec_test.go | 49 +++++++++++++++++++++++++++++++++++++
 pkg/col/coldata/vec_tmpl.go | 10 +++++++-
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/pkg/col/coldata/vec_test.go b/pkg/col/coldata/vec_test.go
index 16a5328813b8..6b8cf282f59d 100644
--- a/pkg/col/coldata/vec_test.go
+++ b/pkg/col/coldata/vec_test.go
@@ -359,6 +359,55 @@ func TestCopyNulls(t *testing.T) {
 	}
 }
 
+func TestCopySelOnDestDoesNotUnsetOldNulls(t *testing.T) {
+	const typ = coltypes.Int64
+
+	// Set up the destination vector. It is all nulls except for a single
+	// non-null at index 0.
+	dst := NewMemColumn(typ, BatchSize)
+	dstInts := dst.Int64()
+	for i := range dstInts {
+		dstInts[i] = 1
+	}
+	dst.Nulls().SetNulls()
+	dst.Nulls().UnsetNull(0)
+
+	// Set up the source vector with two nulls.
+	src := NewMemColumn(typ, BatchSize)
+	srcInts := src.Int64()
+	for i := range srcInts {
+		srcInts[i] = 2
+	}
+	src.Nulls().SetNull(0)
+	src.Nulls().SetNull(3)
+
+	// Using a small selection vector and SelOnDest, perform a copy and verify
+	// that nulls in between the selected tuples weren't unset.
+	copyArgs := CopySliceArgs{
+		SelOnDest: true,
+		SliceArgs: SliceArgs{
+			ColType:     typ,
+			Src:         src,
+			SrcStartIdx: 1,
+			SrcEndIdx:   3,
+			Sel:         []uint16{0, 1, 3},
+		},
+	}
+
+	dst.Copy(copyArgs)
+
+	// 0 was not null in dest and null in source, but it wasn't selected. Not null.
+	require.False(t, dst.Nulls().NullAt(0))
+	// 1 was null in dest and not null in source: it becomes not null.
+	require.False(t, dst.Nulls().NullAt(1))
+	// 2 wasn't included in the selection vector: it stays null.
+	require.True(t, dst.Nulls().NullAt(2))
+	// 3 was null in dest and null in source: it stays null.
+	require.True(t, dst.Nulls().NullAt(3))
+	// 4 wasn't included: it stays null.
+	require.True(t, dst.Nulls().NullAt(4))
+}
+
 func BenchmarkAppend(b *testing.B) {
 	const typ = coltypes.Int64
 
diff --git a/pkg/col/coldata/vec_tmpl.go b/pkg/col/coldata/vec_tmpl.go
index b388a482c07c..2d6221ef49bf 100644
--- a/pkg/col/coldata/vec_tmpl.go
+++ b/pkg/col/coldata/vec_tmpl.go
@@ -93,6 +93,7 @@ func _COPY_WITH_SEL(
 			} else {
 				v := execgen.UNSAFEGET(fromCol, int(selIdx))
 				// {{if .SelOnDest}}
+				m.nulls.UnsetNull64(uint64(selIdx))
 				execgen.SET(toCol, int(selIdx), v)
 				// {{else}}
 				execgen.SET(toCol, i+int(args.DestIdx), v)
@@ -118,7 +119,14 @@ func _COPY_WITH_SEL(
 // */}}
 
 func (m *memColumn) Copy(args CopySliceArgs) {
-	m.Nulls().UnsetNullRange(args.DestIdx, args.DestIdx+(args.SrcEndIdx-args.SrcStartIdx))
+	if !args.SelOnDest {
+		// We're about to overwrite this entire range, so unset all the nulls.
+		m.Nulls().UnsetNullRange(args.DestIdx, args.DestIdx+(args.SrcEndIdx-args.SrcStartIdx))
+	}
+	// } else {
+	// SelOnDest indicates that we're applying the input selection vector as a lens
+	// into the output vector as well. We'll set the non-nulls by hand below.
+	// }
 
 	switch args.ColType {
 	// {{range .}}