sql: add unit tests for planning inside the new schema changer

Previously, the new schema changer did not have any unit
tests covering the planning capability. This was inadequate,
because we had no way of detect if plans were regressing with
changes or enhancements. To address this, this patch adds
basic tests to see if the operators/dependencies for a given
command are sane.
Release note: None

pkg/sql/schemachanger/scgraphviz/graphviz.go

@@ -161,11 +161,11 @@ func htmlLabel(o interface{}) dot.HTML {
 	return dot.HTML(buf.String())
 }
 
-// toMap converts a struct to a map, field by field. If at any point a protobuf
+// ToMap converts a struct to a map, field by field. If at any point a protobuf
 // message is encountered, it is converted to a map using jsonpb to marshal it
 // to json and then marshaling it back to a map. This approach allows zero
 // values to be effectively omitted.
-func toMap(v interface{}) (interface{}, error) {
+func ToMap(v interface{}) (interface{}, error) {
 	if v == nil {
 		return nil, nil
 	}
@@ -202,7 +202,7 @@ func toMap(v interface{}) (interface{}, error) {
 			continue
 		}
 		var err error
-		if m[vt.Field(i).Name], err = toMap(vvf.Interface()); err != nil {
+		if m[vt.Field(i).Name], err = ToMap(vvf.Interface()); err != nil {
 			return nil, err
 		}
 	}
@@ -235,7 +235,7 @@ var objectTemplate = template.Must(template.New("obj").Funcs(template.FuncMap{
 	"isStruct": func(v interface{}) bool {
 		return reflect.Indirect(reflect.ValueOf(v)).Kind() == reflect.Struct
 	},
-	"toMap": toMap,
+	"toMap": ToMap,
 }).Parse(`
 {{- define "key" -}}
 <td>

pkg/sql/schemachanger/scpb/attribute.go

@@ -61,7 +61,7 @@ func (id IndexID) String() string {
 
 // String implements AttributeValue.
 func (id ElementTypeID) String() string {
-	return strconv.Itoa(int(id))
+	return ElementIDToString(id)
 }
 
 // String implements AttributeValue.
@@ -146,16 +146,20 @@ func (a Attributes) Equal(other Attributes) bool {
 // String seralizes attribute into a string
 func (a Attributes) String() string {
 	result := strings.Builder{}
-	result.WriteString("[ ")
+	result.WriteString(a.Get(AttributeType).String())
+	result.WriteString(":{")
 	for attribIdx, attrib := range a.values {
+		if attrib.key == AttributeType {
+			continue
+		}
 		result.WriteString(attrib.key.String())
 		result.WriteString(": ")
 		result.WriteString(attrib.value.String())
 		if attribIdx < len(a.values)-1 {
 			result.WriteString(", ")
 		}
 	}
-	result.WriteString(" ]")
+	result.WriteString("}")
 	return result.String()
 }
 

pkg/sql/schemachanger/scpb/attribute_test.go

@@ -30,22 +30,22 @@ func TestGetAttribute(t *testing.T) {
 
 	// Sanity: Validate basic string conversion, equality,
 	// and inequality.
-	expectedStr := `[ Type: 4, DescID: 3, DepID: 1, ColumnID: 2 ]`
+	expectedStr := `SequenceDependency:{DescID: 3, DepID: 1, ColumnID: 2}`
 	require.Equal(t, expectedStr, seqElem.GetAttributes().String(), "Attribute string conversion is broken.")
 	require.True(t, seqElem.GetAttributes().Equal(seqElem.GetAttributes()))
 	require.False(t, seqElem.GetAttributes().Equal(seqElemDiff.GetAttributes()))
 
 	// Sanity: Validate type references, then check if type comparisons
 	// work.
 	typeBackRef := TypeReference{TypeID: 3, DescID: 1}
-	expectedStr = `[ Type: 10, DescID: 3, DepID: 1 ]`
+	expectedStr = `TypeReference:{DescID: 3, DepID: 1}`
 	require.Equal(t, expectedStr, typeBackRef.GetAttributes().String(), "Attribute string conversion is broken.")
 	require.False(t, seqElem.GetAttributes().Equal(typeBackRef.GetAttributes()))
 	require.False(t, typeBackRef.GetAttributes().Equal(seqElem.GetAttributes()))
 
 	// Sanity: Validate attribute fetching for both types.
 	require.Equal(t, "1", typeBackRef.GetAttributes().Get(AttributeDepID).String())
 	require.Equal(t, "3", typeBackRef.GetAttributes().Get(AttributeDescID).String())
-	require.Equal(t, "10", typeBackRef.GetAttributes().Get(AttributeType).String())
+	require.Equal(t, "TypeReference", typeBackRef.GetAttributes().Get(AttributeType).String())
 	require.Equal(t, "4", seqElemDiff.GetAttributes().Get(AttributeColumnID).String())
 }

pkg/sql/schemachanger/scpb/elements.go

@@ -63,10 +63,12 @@ func NewTarget(dir Target_Direction, elem Element) *Target {
 type ElementTypeID int
 
 var typeToElementID map[reflect.Type]ElementTypeID
+var elementIDToString map[ElementTypeID]string
 
 func init() {
 	typ := reflect.TypeOf((*ElementProto)(nil)).Elem()
 	typeToElementID = make(map[reflect.Type]ElementTypeID, typ.NumField())
+	elementIDToString = make(map[ElementTypeID]string, typ.NumField())
 	for i := 0; i < typ.NumField(); i++ {
 		f := typ.Field(i)
 		protoFlags := strings.Split(f.Tag.Get("protobuf"), ",")
@@ -75,6 +77,7 @@ func init() {
 			panic(errors.Wrapf(err, "failed to extract ID from protobuf tag: %q", protoFlags))
 		}
 		typeToElementID[f.Type] = ElementTypeID(id)
+		elementIDToString[ElementTypeID(id)] = strings.ReplaceAll(f.Type.String(), "*scpb.", "")
 	}
 }
 
@@ -83,6 +86,11 @@ func ElementType(el Element) ElementTypeID {
 	return typeToElementID[reflect.TypeOf(el)]
 }
 
+// ElementIDToString determines the type ID of a element
+func ElementIDToString(id ElementTypeID) string {
+	return elementIDToString[id]
+}
+
 // DescriptorID implements the Element interface.
 func (e *Column) DescriptorID() descpb.ID { return e.TableID }
 

pkg/sql/schemachanger/scplan/BUILD.bazel

@@ -1,4 +1,4 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "scplan",
@@ -21,3 +21,40 @@ go_library(
         "@com_github_cockroachdb_errors//:errors",
     ],
 )
+
+go_test(
+    name = "scplan_test",
+    srcs = [
+        "main_test.go",
+        "plan_test.go",
+    ],
+    data = glob(["testdata/**"]),
+    deps = [
+        ":scplan",
+        "//pkg/base",
+        "//pkg/kv",
+        "//pkg/security",
+        "//pkg/security/securitytest",
+        "//pkg/server",
+        "//pkg/sql",
+        "//pkg/sql/catalog/descpb",
+        "//pkg/sql/catalog/descs",
+        "//pkg/sql/catalog/resolver",
+        "//pkg/sql/parser",
+        "//pkg/sql/schemachanger/scbuild",
+        "//pkg/sql/schemachanger/scgraph",
+        "//pkg/sql/schemachanger/scgraphviz",
+        "//pkg/sql/schemachanger/scpb",
+        "//pkg/sql/sem/tree",
+        "//pkg/sql/sessiondatapb",
+        "//pkg/testutils/serverutils",
+        "//pkg/testutils/sqlutils",
+        "//pkg/testutils/testcluster",
+        "//pkg/util/leaktest",
+        "//pkg/util/log",
+        "//pkg/util/randutil",
+        "@com_github_cockroachdb_datadriven//:datadriven",
+        "@com_github_stretchr_testify//require",
+        "@in_gopkg_yaml_v2//:yaml_v2",
+    ],
+)

pkg/sql/schemachanger/scplan/main_test.go

@@ -0,0 +1,31 @@
+// Copyright 2021 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 scplan_test
+
+import (
+	"os"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/security"
+	"github.com/cockroachdb/cockroach/pkg/security/securitytest"
+	"github.com/cockroachdb/cockroach/pkg/server"
+	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
+	"github.com/cockroachdb/cockroach/pkg/util/randutil"
+)
+
+func TestMain(m *testing.M) {
+	security.SetAssetLoader(securitytest.EmbeddedAssets)
+	randutil.SeedForTests()
+	serverutils.InitTestServerFactory(server.TestServerFactory)
+	serverutils.InitTestClusterFactory(testcluster.TestClusterFactory)
+	os.Exit(m.Run())
+}

pkg/sql/schemachanger/scplan/plan.go

@@ -56,6 +56,9 @@ type Params struct {
 	//
 	// This doesn't do anything right now.
 	CreatedDescriptorIDs catalog.DescriptorIDSet
+	// DisableOpRandomization disables randomization for the final set of
+	// operations.
+	DisableOpRandomization bool
 }
 
 // A Plan is a schema change plan, primarily containing ops to be executed that
@@ -110,7 +113,7 @@ func MakePlan(initialStates []*scpb.Node, params Params) (_ Plan, err error) {
 	}); err != nil {
 		return Plan{}, err
 	}
-	stages := buildStages(initialStates, g)
+	stages := buildStages(initialStates, g, params)
 	return Plan{
 		Params:       params,
 		InitialNodes: initialStates,
@@ -119,7 +122,7 @@ func MakePlan(initialStates []*scpb.Node, params Params) (_ Plan, err error) {
 	}, nil
 }
 
-func buildStages(init []*scpb.Node, g *scgraph.Graph) []Stage {
+func buildStages(init []*scpb.Node, g *scgraph.Graph, params Params) []Stage {
 	// TODO(ajwerner): deal with the case where the target state was
 	// fulfilled by something that preceded the initial state.
 	cur := init
@@ -236,12 +239,15 @@ func buildStages(init []*scpb.Node, g *scgraph.Graph) []Stage {
 		// be order independent, however we will
 		// try to execute non-failing ones first.
 		opsSlice := s.Ops.Slice()
-		rand.Seed(timeutil.Now().UnixNano())
-		rand.Shuffle(len(opsSlice), func(i, j int) {
-			tmp := opsSlice[i]
-			opsSlice[i] = opsSlice[j]
-			opsSlice[j] = tmp
-		})
+		if !params.DisableOpRandomization {
+
+			rand.Seed(timeutil.Now().UnixNano())
+			rand.Shuffle(len(opsSlice), func(i, j int) {
+				tmp := opsSlice[i]
+				opsSlice[i] = opsSlice[j]
+				opsSlice[j] = tmp
+			})
+		}
 		// Place non-revertible operations at the end
 		sort.SliceStable(opsSlice, func(i, j int) bool {
 			if opsSlice[i].Revertible() == opsSlice[j].Revertible() {
@@ -251,6 +257,7 @@ func buildStages(init []*scpb.Node, g *scgraph.Graph) []Stage {
 		})
 		stages = append(stages, s)
 		cur = s.After
+
 	}
 	return stages
 }