vindexes: return unknown params (#12951)

* go/vt/vtgate/vindexes: (in)validate unknown and required params Signed-off-by: Max Englander <max@planetscale.com> * dont mess around with require params for now Signed-off-by: Max Englander <max@planetscale.com> * expose unknown vindex params as warnings Signed-off-by: Max Englander <max@planetscale.com> * fix some broken tests Signed-off-by: Max Englander <max@planetscale.com> * fix more broken tests Signed-off-by: Max Englander <max@planetscale.com> * check for warnings Signed-off-by: Max Englander <max@planetscale.com> * unexport vindexes.New*, force use CreateVindex Signed-off-by: Max Englander <max@planetscale.com> * test for allowed/unknown params Signed-off-by: Max Englander <max@planetscale.com> * checkpoint Signed-off-by: Max Englander <max@planetscale.com> * checkpoint Signed-off-by: Max Englander <max@planetscale.com> * checkpoint Signed-off-by: Max Englander <max@planetscale.com> * checkpoint Signed-off-by: Max Englander <max@planetscale.com> * checkpoint Signed-off-by: Max Englander <max@planetscale.com> * checkpoint Signed-off-by: Max Englander <max@planetscale.com> * checkpoint Signed-off-by: Max Englander <max@planetscale.com> * remove Info() tests Signed-off-by: Max Englander <max@planetscale.com> * checkpoint Signed-off-by: Max Englander <max@planetscale.com> * check for warnings in tests Signed-off-by: Max Englander <max@planetscale.com> * check for warnings in tests Signed-off-by: Max Englander <max@planetscale.com> * rfc option 1 Signed-off-by: Max Englander <max@planetscale.com> * InvalidParamErrors() []error => UnknownParams() []string Signed-off-by: Max Englander <max@planetscale.com> * pr feedback: +license, simplify ret val, constant param keys Signed-off-by: Max Englander <max@planetscale.com> * whoops actually use param constants Signed-off-by: Max Englander <max@planetscale.com> --------- Signed-off-by: Max Englander <max@planetscale.com>
vitessio · Jun 13, 2023 · 7830175 · 7830175
1 parent cc380c4
commit 7830175
Show file tree

Hide file tree

Showing 52 changed files with 2,488 additions and 680 deletions.
diff --git a/go/vt/vtgate/engine/delete_test.go b/go/vt/vtgate/engine/delete_test.go
@@ -64,7 +64,7 @@ func TestDeleteUnsharded(t *testing.T) {
 }
 
 func TestDeleteEqual(t *testing.T) {
-	vindex, _ := vindexes.NewHash("", nil)
+	vindex, _ := vindexes.CreateVindex("hash", "", nil)
 	del := &Delete{
 		DML: &DML{
 			RoutingParameters: &RoutingParameters{
@@ -96,7 +96,7 @@ func TestDeleteEqual(t *testing.T) {
 }
 
 func TestDeleteEqualMultiCol(t *testing.T) {
-	vindex, _ := vindexes.NewRegionExperimental("", map[string]string{"region_bytes": "1"})
+	vindex, _ := vindexes.CreateVindex("region_experimental", "", map[string]string{"region_bytes": "1"})
 	del := &Delete{
 		DML: &DML{
 			RoutingParameters: &RoutingParameters{
@@ -128,7 +128,7 @@ func TestDeleteEqualMultiCol(t *testing.T) {
 }
 
 func TestDeleteEqualNoRoute(t *testing.T) {
-	vindex, _ := vindexes.NewLookupUnique("", map[string]string{
+	vindex, _ := vindexes.CreateVindex("lookup_unique", "", map[string]string{
 		"table": "lkp",
 		"from":  "from",
 		"to":    "toc",
@@ -160,7 +160,7 @@ func TestDeleteEqualNoRoute(t *testing.T) {
 
 func TestDeleteEqualNoScatter(t *testing.T) {
 	t.Skip("planner does not produces this plan anymore")
-	vindex, _ := vindexes.NewLookupUnique("", map[string]string{
+	vindex, _ := vindexes.CreateVindex("lookup_unique", "", map[string]string{
 		"table":      "lkp",
 		"from":       "from",
 		"to":         "toc",
@@ -555,7 +555,7 @@ func TestDeleteInChangedVindexMultiCol(t *testing.T) {
 }
 
 func TestDeleteEqualSubshard(t *testing.T) {
-	vindex, _ := vindexes.NewRegionExperimental("", map[string]string{"region_bytes": "1"})
+	vindex, _ := vindexes.CreateVindex("region_experimental", "", map[string]string{"region_bytes": "1"})
 	del := &Delete{
 		DML: &DML{
 			RoutingParameters: &RoutingParameters{

diff --git a/go/vt/vtgate/engine/plan_description_test.go b/go/vt/vtgate/engine/plan_description_test.go
@@ -50,7 +50,7 @@ func TestCreateRoutePlanDescription(t *testing.T) {
 }
 
 func createRoute() *Route {
-	hash, _ := vindexes.NewHash("vindex name", nil)
+	hash, _ := vindexes.CreateVindex("hash", "vindex name", nil)
 	return &Route{
 		RoutingParameters: &RoutingParameters{
 			Opcode:            Scatter,

diff --git a/go/vt/vtgate/engine/route_test.go b/go/vt/vtgate/engine/route_test.go
@@ -236,7 +236,7 @@ func TestSelectScatter(t *testing.T) {
 }
 
 func TestSelectEqualUnique(t *testing.T) {
-	vindex, _ := vindexes.NewHash("", nil)
+	vindex, _ := vindexes.CreateVindex("hash", "", nil)
 	sel := NewRoute(
 		EqualUnique,
 		&vindexes.Keyspace{
@@ -274,7 +274,7 @@ func TestSelectEqualUnique(t *testing.T) {
 }
 
 func TestSelectNone(t *testing.T) {
-	vindex, _ := vindexes.NewHash("", nil)
+	vindex, _ := vindexes.CreateVindex("hash", "", nil)
 	sel := NewRoute(
 		None,
 		&vindexes.Keyspace{
@@ -325,7 +325,7 @@ func TestSelectNone(t *testing.T) {
 }
 
 func TestSelectEqualUniqueScatter(t *testing.T) {
-	vindex, _ := vindexes.NewLookupUnique("", map[string]string{
+	vindex, _ := vindexes.CreateVindex("lookup_unique", "", map[string]string{
 		"table":      "lkp",
 		"from":       "from",
 		"to":         "toc",
@@ -368,7 +368,7 @@ func TestSelectEqualUniqueScatter(t *testing.T) {
 }
 
 func TestSelectEqual(t *testing.T) {
-	vindex, _ := vindexes.NewLookup("", map[string]string{
+	vindex, _ := vindexes.CreateVindex("lookup", "", map[string]string{
 		"table": "lkp",
 		"from":  "from",
 		"to":    "toc",
@@ -421,7 +421,7 @@ func TestSelectEqual(t *testing.T) {
 }
 
 func TestSelectEqualNoRoute(t *testing.T) {
-	vindex, _ := vindexes.NewLookupUnique("", map[string]string{
+	vindex, _ := vindexes.CreateVindex("lookup_unique", "", map[string]string{
 		"table": "lkp",
 		"from":  "from",
 		"to":    "toc",
@@ -485,7 +485,7 @@ func TestSelectEqualNoRoute(t *testing.T) {
 }
 
 func TestINUnique(t *testing.T) {
-	vindex, _ := vindexes.NewHash("", nil)
+	vindex, _ := vindexes.CreateVindex("hash", "", nil)
 	sel := NewRoute(
 		IN,
 		&vindexes.Keyspace{
@@ -530,7 +530,7 @@ func TestINUnique(t *testing.T) {
 }
 
 func TestINNonUnique(t *testing.T) {
-	vindex, _ := vindexes.NewLookup("", map[string]string{
+	vindex, _ := vindexes.CreateVindex("lookup", "", map[string]string{
 		"table": "lkp",
 		"from":  "from",
 		"to":    "toc",
@@ -597,7 +597,7 @@ func TestINNonUnique(t *testing.T) {
 }
 
 func TestMultiEqual(t *testing.T) {
-	vindex, _ := vindexes.NewHash("", nil)
+	vindex, _ := vindexes.CreateVindex("hash", "", nil)
 	sel := NewRoute(
 		MultiEqual,
 		&vindexes.Keyspace{
@@ -640,7 +640,7 @@ func TestMultiEqual(t *testing.T) {
 }
 
 func TestSelectLike(t *testing.T) {
-	subshard, _ := vindexes.NewCFC("cfc", map[string]string{"hash": "md5", "offsets": "[1,2]"})
+	subshard, _ := vindexes.CreateVindex("cfc", "cfc", map[string]string{"hash": "md5", "offsets": "[1,2]"})
 	vindex := subshard.(*vindexes.CFC).PrefixVindex()
 	vc := &loggingVCursor{
 		// we have shards '-0c80', '0c80-0d', '0d-40', '40-80', '80-'
@@ -816,7 +816,7 @@ func TestSelectReference(t *testing.T) {
 }
 
 func TestRouteGetFields(t *testing.T) {
-	vindex, _ := vindexes.NewLookupUnique("", map[string]string{
+	vindex, _ := vindexes.CreateVindex("lookup_unique", "", map[string]string{
 		"table": "lkp",
 		"from":  "from",
 		"to":    "toc",
@@ -1452,7 +1452,7 @@ func TestExecFail(t *testing.T) {
 }
 
 func TestSelectEqualUniqueMultiColumnVindex(t *testing.T) {
-	vindex, _ := vindexes.NewRegionExperimental("", map[string]string{"region_bytes": "1"})
+	vindex, _ := vindexes.CreateVindex("region_experimental", "", map[string]string{"region_bytes": "1"})
 	sel := NewRoute(
 		EqualUnique,
 		&vindexes.Keyspace{
@@ -1491,7 +1491,7 @@ func TestSelectEqualUniqueMultiColumnVindex(t *testing.T) {
 }
 
 func TestSelectEqualMultiColumnVindex(t *testing.T) {
-	vindex, _ := vindexes.NewRegionExperimental("", map[string]string{"region_bytes": "1"})
+	vindex, _ := vindexes.CreateVindex("region_experimental", "", map[string]string{"region_bytes": "1"})
 	vc := &loggingVCursor{
 		shards:       []string{"-20", "20-"},
 		shardForKsid: []string{"-20", "20-"},
@@ -1528,7 +1528,7 @@ func TestSelectEqualMultiColumnVindex(t *testing.T) {
 }
 
 func TestINMultiColumnVindex(t *testing.T) {
-	vindex, _ := vindexes.NewRegionExperimental("", map[string]string{"region_bytes": "1"})
+	vindex, _ := vindexes.CreateVindex("region_experimental", "", map[string]string{"region_bytes": "1"})
 	sel := NewRoute(
 		IN,
 		&vindexes.Keyspace{
@@ -1574,7 +1574,7 @@ func TestINMultiColumnVindex(t *testing.T) {
 }
 
 func TestINMixedMultiColumnComparision(t *testing.T) {
-	vindex, _ := vindexes.NewRegionExperimental("", map[string]string{"region_bytes": "1"})
+	vindex, _ := vindexes.CreateVindex("region_experimental", "", map[string]string{"region_bytes": "1"})
 	sel := NewRoute(
 		IN,
 		&vindexes.Keyspace{
@@ -1617,7 +1617,7 @@ func TestINMixedMultiColumnComparision(t *testing.T) {
 }
 
 func TestMultiEqualMultiCol(t *testing.T) {
-	vindex, _ := vindexes.NewRegionExperimental("", map[string]string{"region_bytes": "1"})
+	vindex, _ := vindexes.CreateVindex("region_experimental", "", map[string]string{"region_bytes": "1"})
 	sel := NewRoute(
 		MultiEqual,
 		&vindexes.Keyspace{Name: "ks", Sharded: true},

diff --git a/go/vt/vtgate/engine/update_test.go b/go/vt/vtgate/engine/update_test.go
@@ -68,7 +68,7 @@ func TestUpdateUnsharded(t *testing.T) {
 }
 
 func TestUpdateEqual(t *testing.T) {
-	vindex, _ := vindexes.NewHash("", nil)
+	vindex, _ := vindexes.CreateVindex("hash", "", nil)
 	upd := &Update{
 		DML: &DML{
 			RoutingParameters: &RoutingParameters{
@@ -99,7 +99,7 @@ func TestUpdateEqual(t *testing.T) {
 }
 
 func TestUpdateEqualMultiCol(t *testing.T) {
-	vindex, _ := vindexes.NewRegionExperimental("", map[string]string{"region_bytes": "1"})
+	vindex, _ := vindexes.CreateVindex("region_experimental", "", map[string]string{"region_bytes": "1"})
 	upd := &Update{
 		DML: &DML{
 			RoutingParameters: &RoutingParameters{
@@ -125,7 +125,7 @@ func TestUpdateEqualMultiCol(t *testing.T) {
 }
 
 func TestUpdateScatter(t *testing.T) {
-	vindex, _ := vindexes.NewHash("", nil)
+	vindex, _ := vindexes.CreateVindex("hash", "", nil)
 	upd := &Update{
 		DML: &DML{
 			RoutingParameters: &RoutingParameters{
@@ -178,7 +178,7 @@ func TestUpdateScatter(t *testing.T) {
 }
 
 func TestUpdateEqualNoRoute(t *testing.T) {
-	vindex, _ := vindexes.NewLookupUnique("", map[string]string{
+	vindex, _ := vindexes.CreateVindex("lookup_unique", "", map[string]string{
 		"table": "lkp",
 		"from":  "from",
 		"to":    "toc",
@@ -210,7 +210,7 @@ func TestUpdateEqualNoRoute(t *testing.T) {
 
 func TestUpdateEqualNoScatter(t *testing.T) {
 	t.Skip("planner does not produces this plan anymore")
-	vindex, _ := vindexes.NewLookupUnique("", map[string]string{
+	vindex, _ := vindexes.CreateVindex("lookup_unique", "", map[string]string{
 		"table":      "lkp",
 		"from":       "from",
 		"to":         "toc",
@@ -890,7 +890,7 @@ func TestUpdateInChangedVindexMultiCol(t *testing.T) {
 }
 
 func TestUpdateEqualSubshard(t *testing.T) {
-	vindex, _ := vindexes.NewRegionExperimental("", map[string]string{"region_bytes": "1"})
+	vindex, _ := vindexes.CreateVindex("region_experimental", "", map[string]string{"region_bytes": "1"})
 	upd := &Update{
 		DML: &DML{
 			RoutingParameters: &RoutingParameters{

diff --git a/go/vt/vtgate/planbuilder/from.go b/go/vt/vtgate/planbuilder/from.go
@@ -285,7 +285,7 @@ func (pb *primitiveBuilder) buildTablePrimitive(tableExpr *sqlparser.AliasedTabl
 		// Use the Binary vindex, which is the identity function
 		// for keyspace id.
 		eroute = engine.NewSimpleRoute(engine.EqualUnique, vschemaTable.Keyspace)
-		vindex, _ = vindexes.NewBinary("binary", nil)
+		vindex, _ = vindexes.CreateVindex("binary", "binary", nil)
 		eroute.Vindex = vindex
 		lit := evalengine.NewLiteralString(vschemaTable.Pinned, collations.TypedCollation{})
 		eroute.Values = []evalengine.Expr{lit}

diff --git a/go/vt/vtgate/planbuilder/operators/sharded_routing.go b/go/vt/vtgate/planbuilder/operators/sharded_routing.go
@@ -64,7 +64,7 @@ func newShardedRouting(vtable *vindexes.Table, id semantics.TableSet) Routing {
 		// Use the Binary vindex, which is the identity function
 		// for keyspace id.
 		routing.RouteOpCode = engine.EqualUnique
-		vindex, _ := vindexes.NewBinary("binary", nil)
+		vindex, _ := vindexes.CreateVindex("binary", "binary", nil)
 		routing.Selected = &VindexOption{
 			Ready:       true,
 			Values:      []evalengine.Expr{evalengine.NewLiteralString(vtable.Pinned, collations.TypedCollation{})},

diff --git a/go/vt/vtgate/semantics/analyzer_test.go b/go/vt/vtgate/semantics/analyzer_test.go
@@ -1186,7 +1186,7 @@ func TestScopingWVindexTables(t *testing.T) {
 		t.Run(query.query, func(t *testing.T) {
 			parse, err := sqlparser.Parse(query.query)
 			require.NoError(t, err)
-			hash, _ := vindexes.NewHash("user_index", nil)
+			hash, _ := vindexes.CreateVindex("hash", "user_index", nil)
 			st, err := Analyze(parse, "user", &FakeSI{
 				Tables: map[string]*vindexes.Table{
 					"t": {Name: sqlparser.NewIdentifierCS("t")},

diff --git a/go/vt/vtgate/vindexes/binary.go b/go/vt/vtgate/vindexes/binary.go
@@ -26,19 +26,24 @@ import (
 )
 
 var (
-	_ SingleColumn = (*Binary)(nil)
-	_ Reversible   = (*Binary)(nil)
-	_ Hashing      = (*Binary)(nil)
+	_ SingleColumn    = (*Binary)(nil)
+	_ Reversible      = (*Binary)(nil)
+	_ Hashing         = (*Binary)(nil)
+	_ ParamValidating = (*Binary)(nil)
 )
 
 // Binary is a vindex that converts binary bits to a keyspace id.
 type Binary struct {
-	name string
+	name          string
+	unknownParams []string
 }
 
-// NewBinary creates a new Binary.
-func NewBinary(name string, _ map[string]string) (Vindex, error) {
-	return &Binary{name: name}, nil
+// newBinary creates a new Binary.
+func newBinary(name string, params map[string]string) (Vindex, error) {
+	return &Binary{
+		name:          name,
+		unknownParams: FindUnknownParams(params, nil),
+	}, nil
 }
 
 // String returns the name of the vindex.
@@ -103,6 +108,11 @@ func (*Binary) ReverseMap(_ VCursor, ksids [][]byte) ([]sqltypes.Value, error) {
 	return reverseIds, nil
 }
 
+// UnknownParams implements the ParamValidating interface.
+func (vind *Binary) UnknownParams() []string {
+	return vind.unknownParams
+}
+
 func init() {
-	Register("binary", NewBinary)
+	Register("binary", newBinary)
 }
diff --git a/go/vt/vtgate/vindexes/binary_test.go b/go/vt/vtgate/vindexes/binary_test.go
@@ -24,7 +24,6 @@ import (
 	"reflect"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"vitess.io/vitess/go/sqltypes"
@@ -34,15 +33,58 @@ import (
 var binOnlyVindex SingleColumn
 
 func init() {
-	vindex, _ := CreateVindex("binary", "binary_varchar", nil)
+	vindex, err := CreateVindex("binary", "binary_varchar", nil)
+	if err != nil {
+		panic(err)
+	}
 	binOnlyVindex = vindex.(SingleColumn)
 }
 
-func TestBinaryInfo(t *testing.T) {
-	assert.Equal(t, 0, binOnlyVindex.Cost())
-	assert.Equal(t, "binary_varchar", binOnlyVindex.String())
-	assert.True(t, binOnlyVindex.IsUnique())
-	assert.False(t, binOnlyVindex.NeedsVCursor())
+func binaryCreateVindexTestCase(
+	testName string,
+	vindexParams map[string]string,
+	expectErr error,
+	expectUnknownParams []string,
+) createVindexTestCase {
+	return createVindexTestCase{
+		testName: testName,
+
+		vindexType:   "binary",
+		vindexName:   "binary",
+		vindexParams: vindexParams,
+
+		expectCost:          0,
+		expectErr:           expectErr,
+		expectIsUnique:      true,
+		expectNeedsVCursor:  false,
+		expectString:        "binary",
+		expectUnknownParams: expectUnknownParams,
+	}
+}
+
+func TestBinaryCreateVindex(t *testing.T) {
+	cases := []createVindexTestCase{
+		binaryCreateVindexTestCase(
+			"no params",
+			nil,
+			nil,
+			nil,
+		),
+		binaryCreateVindexTestCase(
+			"empty params",
+			map[string]string{},
+			nil,
+			nil,
+		),
+		binaryCreateVindexTestCase(
+			"unknown params",
+			map[string]string{"hello": "world"},
+			nil,
+			[]string{"hello"},
+		),
+	}
+
+	testCreateVindexes(t, cases)
 }
 
 func TestBinaryMap(t *testing.T) {