[release-20.0] LookupVindex: fix CLI to allow creating non-unique lookups with single column (#17301)

go/test/endtoend/vreplication/lookupindex_helper_test.go

@@ -0,0 +1,113 @@
+/*
+Copyright 2024 The Vitess Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package vreplication
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"github.com/tidwall/gjson"
+
+	"vitess.io/vitess/go/vt/sqlparser"
+
+	binlogdatapb "vitess.io/vitess/go/vt/proto/binlogdata"
+)
+
+type lookupIndex struct {
+	typ                string
+	name               string
+	tableKeyspace      string
+	table              string
+	columns            []string
+	ownerTable         string
+	ownerTableKeyspace string
+	ignoreNulls        bool
+
+	t *testing.T
+}
+
+func (li *lookupIndex) String() string {
+	return li.typ + " " + li.name + " on " + li.tableKeyspace + "." + li.table + " (" + li.columns[0] + ")"
+}
+
+func (li *lookupIndex) create() {
+	cols := strings.Join(li.columns, ",")
+	args := []string{
+		"LookupVindex",
+		"--name", li.name,
+		"--table-keyspace=" + li.ownerTableKeyspace,
+		"create",
+		"--keyspace=" + li.tableKeyspace,
+		"--type=" + li.typ,
+		"--table-owner=" + li.ownerTable,
+		"--table-owner-columns=" + cols,
+		"--tablet-types=PRIMARY",
+	}
+	if li.ignoreNulls {
+		args = append(args, "--ignore-nulls")
+	}
+
+	err := vc.VtctldClient.ExecuteCommand(args...)
+	require.NoError(li.t, err, "error executing LookupVindex create: %v", err)
+	waitForWorkflowState(li.t, vc, fmt.Sprintf("%s.%s", li.ownerTableKeyspace, li.name), binlogdatapb.VReplicationWorkflowState_Running.String())
+	li.expectWriteOnly(true)
+}
+
+func (li *lookupIndex) cancel() {
+	panic("not implemented")
+}
+
+func (li *lookupIndex) externalize() {
+	args := []string{
+		"LookupVindex",
+		"--name", li.name,
+		"--table-keyspace=" + li.ownerTableKeyspace,
+		"externalize",
+		"--keyspace=" + li.tableKeyspace,
+	}
+	err := vc.VtctldClient.ExecuteCommand(args...)
+	require.NoError(li.t, err, "error executing LookupVindex externalize: %v", err)
+	li.expectWriteOnly(false)
+}
+
+func (li *lookupIndex) show() error {
+	return nil
+}
+
+func (li *lookupIndex) expectWriteOnly(expected bool) {
+	vschema, err := vc.VtctldClient.ExecuteCommandWithOutput("GetVSchema", li.ownerTableKeyspace)
+	require.NoError(li.t, err, "error executing GetVSchema: %v", err)
+	vdx := gjson.Get(vschema, fmt.Sprintf("vindexes.%s", li.name))
+	require.NotNil(li.t, vdx, "lookup vindex %s not found", li.name)
+	want := ""
+	if expected {
+		want = "true"
+	}
+	require.Equal(li.t, want, vdx.Get("params.write_only").String(), "expected write_only parameter to be %s", want)
+}
+
+func getNumRowsInQuery(t *testing.T, query string) int {
+	stmt, err := sqlparser.NewTestParser().Parse(query)
+	require.NoError(t, err)
+	insertStmt, ok := stmt.(*sqlparser.Insert)
+	require.True(t, ok)
+	rows, ok := insertStmt.Rows.(sqlparser.Values)
+	require.True(t, ok)
+	return len(rows)
+}

go/test/endtoend/vreplication/lookupindex_test.go

@@ -0,0 +1,204 @@
+/*
+Copyright 2024 The Vitess Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package vreplication
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"vitess.io/vitess/go/test/endtoend/cluster"
+	vttablet "vitess.io/vitess/go/vt/vttablet/common"
+)
+
+type TestClusterSpec struct {
+	keyspaceName string
+	vschema      string
+	schema       string
+}
+
+var lookupClusterSpec = TestClusterSpec{
+	keyspaceName: "lookup",
+	vschema: `
+{
+  "sharded": true,
+  "vindexes": {
+    "reverse_bits": {
+      "type": "reverse_bits"
+    }
+  },
+  "tables": {
+	"t1": {
+	      "column_vindexes": [
+	        {
+	          "column": "c1",
+	          "name": "reverse_bits"
+	        }
+	      ]
+	}
+  }
+}
+`,
+	schema: `
+create table t1(
+	c1 int,
+	c2 int,
+	val varchar(128),
+	primary key(c1)
+);
+`,
+}
+
+func setupLookupIndexKeyspace(t *testing.T) map[string]*cluster.VttabletProcess {
+	tablets := make(map[string]*cluster.VttabletProcess)
+	if _, err := vc.AddKeyspace(t, []*Cell{vc.Cells["zone1"]}, lookupClusterSpec.keyspaceName, "-80,80-",
+		lookupClusterSpec.vschema, lookupClusterSpec.schema, defaultReplicas, defaultRdonly, 200, nil); err != nil {
+		require.NoError(t, err)
+	}
+	defaultCell := vc.Cells[vc.CellNames[0]]
+	ks := vc.Cells[defaultCell.Name].Keyspaces[lookupClusterSpec.keyspaceName]
+	targetTab1 = ks.Shards["-80"].Tablets["zone1-200"].Vttablet
+	targetTab2 = ks.Shards["80-"].Tablets["zone1-300"].Vttablet
+	tablets["-80"] = targetTab1
+	tablets["80-"] = targetTab2
+	return tablets
+}
+
+type lookupTestCase struct {
+	name                 string
+	li                   *lookupIndex
+	initQuery            string
+	runningQuery         string
+	postExternalizeQuery string
+	cleanupQuery         string
+}
+
+func TestLookupIndex(t *testing.T) {
+	setSidecarDBName("_vt")
+	origDefaultReplicas := defaultReplicas
+	origDefaultRdonly := defaultRdonly
+	defer func() {
+		defaultReplicas = origDefaultReplicas
+		defaultRdonly = origDefaultRdonly
+	}()
+	defaultReplicas = 1
+	defaultRdonly = 0
+	vc = setupMinimalCluster(t)
+	defer vc.TearDown()
+	vttablet.InitVReplicationConfigDefaults()
+
+	_ = setupLookupIndexKeyspace(t)
+
+	initQuery := "insert into t1 (c1, c2, val) values (1, 1, 'val1'), (2, 2, 'val2'), (3, 3, 'val3')"
+	runningQuery := "insert into t1 (c1, c2, val) values (4, 4, 'val4'), (5, 5, 'val5'), (6, 6, 'val6')"
+	postExternalizeQuery := "insert into t1 (c1, c2, val) values (7, 7, 'val7'), (8, 8, 'val8'), (9, 9, 'val9')"
+	cleanupQuery := "delete from t1"
+
+	testCases := []lookupTestCase{
+		{
+			name: "non-unique lookup index, one column",
+			li: &lookupIndex{
+				typ:                "consistent_lookup",
+				name:               "t1_c2_lookup",
+				tableKeyspace:      lookupClusterSpec.keyspaceName,
+				table:              "t1",
+				columns:            []string{"c2"},
+				ownerTable:         "t1",
+				ownerTableKeyspace: lookupClusterSpec.keyspaceName,
+				ignoreNulls:        true,
+				t:                  t,
+			},
+		},
+		{
+			name: "lookup index, two columns",
+			li: &lookupIndex{
+				typ:                "lookup",
+				name:               "t1_c2_val_lookup",
+				tableKeyspace:      lookupClusterSpec.keyspaceName,
+				table:              "t1",
+				columns:            []string{"c2", "val"},
+				ownerTable:         "t1",
+				ownerTableKeyspace: lookupClusterSpec.keyspaceName,
+				ignoreNulls:        true,
+				t:                  t,
+			},
+		},
+		{
+			name: "unique lookup index, one column",
+			li: &lookupIndex{
+				typ:                "lookup_unique",
+				name:               "t1_c2_unique_lookup",
+				tableKeyspace:      lookupClusterSpec.keyspaceName,
+				table:              "t1",
+				columns:            []string{"c2"},
+				ownerTable:         "t1",
+				ownerTableKeyspace: lookupClusterSpec.keyspaceName,
+				ignoreNulls:        true,
+				t:                  t,
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			tc.initQuery = initQuery
+			tc.runningQuery = runningQuery
+			tc.postExternalizeQuery = postExternalizeQuery
+			tc.cleanupQuery = cleanupQuery
+			testLookupVindex(t, &tc)
+		})
+	}
+}
+
+func testLookupVindex(t *testing.T, tc *lookupTestCase) {
+	vtgateConn, cancel := getVTGateConn()
+	defer cancel()
+	var totalRows int
+	li := tc.li
+
+	t.Run("init data", func(t *testing.T) {
+		totalRows += getNumRowsInQuery(t, tc.initQuery)
+		_, err := vtgateConn.ExecuteFetch(tc.initQuery, 1000, false)
+		require.NoError(t, err)
+	})
+
+	t.Run("create", func(t *testing.T) {
+		tc.li.create()
+
+		lks := li.tableKeyspace
+		vindexName := li.name
+		waitForRowCount(t, vtgateConn, lks, vindexName, totalRows)
+		totalRows += getNumRowsInQuery(t, tc.runningQuery)
+		_, err := vtgateConn.ExecuteFetch(tc.runningQuery, 1000, false)
+		require.NoError(t, err)
+		waitForRowCount(t, vtgateConn, tc.li.ownerTableKeyspace, li.name, totalRows)
+	})
+
+	t.Run("externalize", func(t *testing.T) {
+		tc.li.externalize()
+		totalRows += getNumRowsInQuery(t, tc.postExternalizeQuery)
+		_, err := vtgateConn.ExecuteFetch(tc.postExternalizeQuery, 1000, false)
+		require.NoError(t, err)
+		waitForRowCount(t, vtgateConn, tc.li.ownerTableKeyspace, li.name, totalRows)
+	})
+
+	t.Run("cleanup", func(t *testing.T) {
+		_, err := vtgateConn.ExecuteFetch(tc.cleanupQuery, 1000, false)
+		require.NoError(t, err)
+		waitForRowCount(t, vtgateConn, tc.li.ownerTableKeyspace, li.name, 0)
+	})
+}

go/vt/vtctl/workflow/materializer_test.go

@@ -2046,7 +2046,7 @@ func TestCreateLookupVindexFailures(t *testing.T) {
 			err: "unique vindex 'from' should have only one column",
 		},
 		{
-			description: "non-unique lookup should have more than one column",
+			description: "non-unique lookup can have only one column",
 			input: &vschemapb.Keyspace{
 				Vindexes: map[string]*vschemapb.Vindex{
 					"v": {
@@ -2059,7 +2059,7 @@ func TestCreateLookupVindexFailures(t *testing.T) {
 					},
 				},
 			},
-			err: "non-unique vindex 'from' should have more than one column",
+			err: "",
 		},
 		{
 			description: "vindex not found",

go/vt/vtctl/workflow/server.go

@@ -3710,10 +3710,13 @@
 		if len(vindexFromCols) != 1 {
 			return nil, nil, nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "unique vindex 'from' should have only one column")
 		}
+<<<<<<< HEAD
 	} else {
 		if len(vindexFromCols) < 2 {
 			return nil, nil, nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "non-unique vindex 'from' should have more than one column")
 		}
+=======
+>>>>>>> 9a03317dcc (LookupVindex: fix CLI to allow creating non-unique lookups with single column (#17301))
 	}
 	vindexToCol = vindex.Params["to"]
 	// Make the vindex write_only. If one exists already in the vschema,
@@ -3753,7 +3756,7 @@
 		// This is a primary vindex definition for the target table
 		// which allows you to override the vindex type used.
 		var vindexCols []string
 		if len(table.ColumnVindexes[0].Columns) != 0 {
 			vindexCols = table.ColumnVindexes[0].Columns
 		} else {
 			if table.ColumnVindexes[0].Column == "" {
@@ -3862,7 +3865,7 @@
 		return nil, nil, nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "schema looks incorrect: %s, expecting at least four lines", tableSchema.TableDefinitions[0].Schema)
 	}
 	var modified []string
 	modified = append(modified, strings.Replace(lines[0], sourceTableName, targetTableName, 1))
 	for i := range sourceVindexColumns {
 		line, err := generateColDef(lines, sourceVindexColumns[i], vindexFromCols[i])
 		if err != nil {
@@ -3927,7 +3930,7 @@
 		// definition if one was not explicitly specified.
 		var targetVindexType string
 		var targetVindex *vschemapb.Vindex
 		for _, field := range tableSchema.TableDefinitions[0].Fields {
 			if sourceVindexColumns[0] == field.Name {
 				if targetTable != nil && len(targetTable.ColumnVindexes) > 0 {
 					targetVindexType = targetTable.ColumnVindexes[0].Name

go/vt/wrangler/materializer.go

@@ -550,11 +550,8 @@ func (wr *Wrangler) prepareCreateLookup(ctx context.Context, keyspace string, sp
 		if len(vindexFromCols) != 1 {
 			return nil, nil, nil, fmt.Errorf("unique vindex 'from' should have only one column: %v", vindex)
 		}
-	} else {
-		if len(vindexFromCols) < 2 {
-			return nil, nil, nil, fmt.Errorf("non-unique vindex 'from' should have more than one column: %v", vindex)
-		}
 	}
+
 	vindexToCol = vindex.Params["to"]
 	// Make the vindex write_only. If one exists already in the vschema,
 	// it will need to match this vindex exactly, including the write_only setting.

go/vt/wrangler/materializer_test.go

@@ -1599,7 +1599,7 @@ func TestCreateLookupVindexFailures(t *testing.T) {
 		},
 		err: "unique vindex 'from' should have only one column",
 	}, {
-		description: "non-unique lookup should have more than one column",
+		description: "non-unique lookup can have only one column",
 		input: &vschemapb.Keyspace{
 			Vindexes: map[string]*vschemapb.Vindex{
 				"v": {
@@ -1612,7 +1612,7 @@ func TestCreateLookupVindexFailures(t *testing.T) {
 				},
 			},
 		},
-		err: "non-unique vindex 'from' should have more than one column",
+		err: "",
 	}, {
 		description: "vindex not found",
 		input: &vschemapb.Keyspace{