Repository: vitessio/vitess
Pull request #6090 (2020-04-18 11:28:42)

Minor refactoring of vtgate plans


Patch:
diff --git a/go/vt/vtgate/engine/send.go b/go/vt/vtgate/engine/send.go
index c812fa53ac..8eab3a8d06 100644
--- a/go/vt/vtgate/engine/send.go
+++ b/go/vt/vtgate/engine/send.go
@@ -73,7 +73,7 @@ func (s *Send) GetTableName() string {
 }
 
 // Execute implements Primitive interface
-func (s *Send) Execute(vcursor VCursor, bindVars map[string]*querypb.BindVariable, wantfields bool) (*sqltypes.Result, error) {
+func (s *Send) Execute(vcursor VCursor, bindVars map[string]*querypb.BindVariable, _ bool) (*sqltypes.Result, error) {
 	rss, _, err := vcursor.ResolveDestinations(s.Keyspace.Name, nil, []key.Destination{s.TargetDestination})
 	if err != nil {
 		return nil, vterrors.Wrap(err, "sendExecute")
@@ -110,43 +110,12 @@ func (s *Send) Execute(vcursor VCursor, bindVars map[string]*querypb.BindVariabl
 }
 
 // StreamExecute implements Primitive interface
-func (s *Send) StreamExecute(vcursor VCursor, bindVars map[string]*querypb.BindVariable, wantfields bool, callback func(*sqltypes.Result) error) error {
-	rss, _, err := vcursor.ResolveDestinations(s.Keyspace.Name, nil, []key.Destination{s.TargetDestination})
-	if err != nil {
-		return vterrors.Wrap(err, "sendStreamExecute")
-	}
-
-	if !s.Keyspace.Sharded && len(rss) != 1 {
-		return vterrors.Errorf(vtrpcpb.Code_FAILED_PRECONDITION, "Keyspace does not have exactly one shard: %v", rss)
-	}
-
-	if s.SingleShardOnly && len(rss) != 1 {
-		return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Unexpected error, DestinationKeyspaceID mapping to multiple shards: %s, got: %v", s.Query, s.TargetDestination)
-	}
-
-	queries := make([]*querypb.BoundQuery, len(rss))
-	for i := range rss {
-		queries[i] = &querypb.BoundQuery{
-			Sql:           s.Query,
-			BindVariables: bindVars,
-		}
-	}
-
-	multiBindVars := make([]map[string]*querypb.BindVariable, len(rss))
-	for i := range multiBindVars {
-		multiBindVars[i] = bindVars
-	}
-	return vcursor.StreamExecuteMulti(s.Query, rss, multiBindVars, callback)
+func (s *Send) StreamExecute(vcursor VCursor, bindVars map[string]*querypb.BindVariable, wantields bool, callback func(*sqltypes.Result) error) error {
+	return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "not reachable") // TODO: systay - this should work
 }
 
 // GetFields implements Primitive interface
 func (s *Send) GetFields(vcursor VCursor, bindVars map[string]*querypb.BindVariable) (*sqltypes.Result, error) {
-	// We don't need to worry about GetFields being needed for joins since the Send primitive currently doesn't
-	// get nested with other types of primitives.
-	// However, GetFields is used for prepared statements. Because of that, prepared statements are not yet
-	// compatible with the Send primitive.
-	//
-	// TODO: implement this
 	return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "not reachable")
 }
 
diff --git a/go/vt/vtgate/executor.go b/go/vt/vtgate/executor.go
index 2b7fc6c85b..6a1652424e 100644
--- a/go/vt/vtgate/executor.go
+++ b/go/vt/vtgate/executor.go
@@ -991,8 +991,7 @@ func (e *Executor) handleComment(sql string) (*sqltypes.Result, error) {
 // StreamExecute executes a streaming query.
 func (e *Executor) StreamExecute(ctx context.Context, method string, safeSession *SafeSession, sql string, bindVars map[string]*querypb.BindVariable, target querypb.Target, callback func(*sqltypes.Result) error) (err error) {
 	logStats := NewLogStats(ctx, method, sql, bindVars)
-	stmtType := sqlparser.Preview(sql)
-	logStats.StmtType = stmtType.String()
+	logStats.StmtType = sqlparser.Preview(sql).String()
 	defer logStats.Send()
 
 	if bindVars == nil {
@@ -1001,22 +1000,10 @@ func (e *Executor) StreamExecute(ctx context.Context, method string, safeSession
 	query, comments := sqlparser.SplitMarginComments(sql)
 	vcursor, _ := newVCursorImpl(ctx, safeSession, comments, e, logStats, e.vm, e.resolver.resolver)
 
-	switch stmtType {
-	case sqlparser.StmtStream:
-		// this is a stream statement for messaging
-		// TODO: support keyRange syntax
+	// check if this is a stream statement for messaging
+	// TODO: support keyRange syntax
+	if logStats.StmtType == sqlparser.StmtStream.String() {
 		return e.handleMessageStream(ctx, sql, target, callback, vcursor, logStats)
-	case sqlparser.StmtSelect, sqlparser.StmtDDL, sqlparser.StmtSet, sqlparser.StmtInsert, sqlparser.StmtReplace, sqlparser.StmtUpdate, sqlparser.StmtDelete,
-		sqlparser.StmtUse, sqlparser.StmtOther, sqlparser.StmtComment:
-		// These may or may not all work, but getPlan() should either return a plan with instructions
-		// or an error, so it's safe to try.
-		break
-	case sqlparser.StmtBegin, sqlparser.StmtCommit, sqlparser.StmtRollback:
-		// These statements don't populate plan.Instructions. We want to make sure we don't try to
-		// dereference nil Instructions which would panic.
-		fallthrough
-	default:
-		return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "unsupported statement type for OLAP: %s", stmtType)
 	}
 
 	plan, err := e.getPlan(
diff --git a/go/vt/vtgate/planbuilder/select.go b/go/vt/vtgate/planbuilder/select.go
index 78ebcf3cc9..9bba8902fa 100644
--- a/go/vt/vtgate/planbuilder/select.go
+++ b/go/vt/vtgate/planbuilder/select.go
@@ -130,47 +130,46 @@ func (pb *primitiveBuilder) processSelect(sel *sqlparser.Select, outer *symtab)
 }
 
 func tryAtVtgate(sel *sqlparser.Select) engine.Primitive {
-	if checkForDual(sel) {
+	dual := isOnlyDual(sel.From)
+	if !dual {
+		return nil
+	}
+
+	exprs := make([]evalengine.Expr, len(sel.SelectExprs))
+	cols := make([]string, len(sel.SelectExprs))
+	for i, e := range sel.SelectExprs {
+		expr, ok := e.(*sqlparser.AliasedExpr)
+		if !ok {
+			return nil
+		}
 		var err error
-		exprs := make([]evalengine.Expr, len(sel.SelectExprs))
-		cols := make([]string, len(sel.SelectExprs))
-		for i, e := range sel.SelectExprs {
-			expr := e.(*sqlparser.AliasedExpr)
-			exprs[i], err = sqlparser.Convert(expr.Expr)
-			if err != nil {
-				return nil
-			}
-			cols[i] = expr.As.String()
+		exprs[i], err = sqlparser.Convert(expr.Expr)
+		if err != nil {
+			return nil
 		}
-		return &engine.Projection{
-			Exprs: exprs,
-			Cols:  cols,
-			Input: &engine.SingleRow{},
+		cols[i] = expr.As.String()
+		if cols[i] == "" {
+			cols[i] = sqlparser.String(expr.Expr)
 		}
 	}
-	return nil
+	return &engine.Projection{
+		Exprs: exprs,
+		Cols:  cols,
+		Input: &engine.SingleRow{},
+	}
 }
 
-func checkForDual(sel *sqlparser.Select) bool {
-	if len(sel.From) == 1 {
-		if from, ok := sel.From[0].(*sqlparser.AliasedTableExpr); ok {
-			if tableName, ok := from.Expr.(sqlparser.TableName); ok {
-				if tableName.Name.String() == "dual" && tableName.Qualifier.IsEmpty() {
-					for _, expr := range sel.SelectExprs {
-						e, ok := expr.(*sqlparser.AliasedExpr)
-						if !ok {
-							return false
-						}
-						if _, ok := e.Expr.(*sqlparser.SQLVal); !ok {
-							return false
-						}
-					}
-					return true
-				}
-			}
-		}
+func isOnlyDual(from sqlparser.TableExprs) bool {
+	if len(from) > 1 {
+		return false
+	}
+	table, ok := from[0].(*sqlparser.AliasedTableExpr)
+	if !ok {
+		return false
 	}
-	return false
+	tableName, ok := table.Expr.(sqlparser.TableName)
+
+	return ok && tableName.Name.String() == "dual" && tableName.Qualifier.IsEmpty()
 }
 
 // pushFilter identifies the target route for the specified bool expr,


Test patch:
diff --git a/go/vt/vtgate/engine/send_test.go b/go/vt/vtgate/engine/send_test.go
index 162018aac2..4604ce0e48 100644
--- a/go/vt/vtgate/engine/send_test.go
+++ b/go/vt/vtgate/engine/send_test.go
@@ -34,9 +34,7 @@ func TestSendTable(t *testing.T) {
 		shards           []string
 		destination      key.Destination
 		expectedQueryLog []string
-		expectedError    string
 		isDML            bool
-		singleShardOnly  bool
 	}
 
 	singleShard := []string{"0"}
@@ -97,18 +95,6 @@ func TestSendTable(t *testing.T) {
 			},
 			isDML: true,
 		},
-		{
-			testName:    "sharded with multi shard destination",
-			sharded:     true,
-			shards:      twoShards,
-			destination: key.DestinationAllShards{},
-			expectedQueryLog: []string{
-				`ResolveDestinations ks [] Destinations:DestinationAllShards()`,
-			},
-			expectedError:   "Unexpected error, DestinationKeyspaceID mapping to multiple shards: dummy_query, got: DestinationAllShards()",
-			isDML:           true,
-			singleShardOnly: true,
-		},
 	}
 
 	for _, tc := range tests {
@@ -121,15 +107,10 @@ func TestSendTable(t *testing.T) {
 				Query:             "dummy_query",
 				TargetDestination: tc.destination,
 				IsDML:             tc.isDML,
-				SingleShardOnly:   tc.singleShardOnly,
 			}
 			vc := &loggingVCursor{shards: tc.shards}
 			_, err := send.Execute(vc, map[string]*querypb.BindVariable{}, false)
-			if tc.expectedError != "" {
-				require.EqualError(t, err, tc.expectedError)
-			} else {
-				require.NoError(t, err)
-			}
+			require.NoError(t, err)
 			vc.ExpectLog(t, tc.expectedQueryLog)
 
 			// Failure cases
@@ -145,122 +126,3 @@ func TestSendTable(t *testing.T) {
 		})
 	}
 }
-
-func TestSendTable_StreamExecute(t *testing.T) {
-	type testCase struct {
-		testName         string
-		sharded          bool
-		shards           []string
-		destination      key.Destination
-		expectedQueryLog []string
-		expectedError    string
-		isDML            bool
-		singleShardOnly  bool
-	}
-
-	singleShard := []string{"0"}
-	twoShards := []string{"-20", "20-"}
-	tests := []testCase{
-		{
-			testName:    "unsharded with no autocommit",
-			sharded:     false,
-			shards:      singleShard,
-			destination: key.DestinationAllShards{},
-			expectedQueryLog: []string{
-				`ResolveDestinations ks [] Destinations:DestinationAllShards()`,
-				`StreamExecuteMulti dummy_query ks.0: {} `,
-			},
-			isDML: false,
-		},
-		{
-			testName:    "sharded with no autocommit",
-			sharded:     true,
-			shards:      twoShards,
-			destination: key.DestinationShard("20-"),
-			expectedQueryLog: []string{
-				`ResolveDestinations ks [] Destinations:DestinationShard(20-)`,
-				`StreamExecuteMulti dummy_query ks.DestinationShard(20-): {} `,
-			},
-			isDML: false,
-		},
-		{
-			testName:    "unsharded",
-			sharded:     false,
-			shards:      singleShard,
-			destination: key.DestinationAllShards{},
-			expectedQueryLog: []string{
-				`ResolveDestinations ks [] Destinations:DestinationAllShards()`,
-				`StreamExecuteMulti dummy_query ks.0: {} `,
-			},
-			isDML: true,
-		},
-		{
-			testName:    "sharded with single shard destination",
-			sharded:     true,
-			shards:      twoShards,
-			destination: key.DestinationShard("20-"),
-			expectedQueryLog: []string{
-				`ResolveDestinations ks [] Destinations:DestinationShard(20-)`,
-				`StreamExecuteMulti dummy_query ks.DestinationShard(20-): {} `,
-			},
-			isDML: true,
-		},
-		{
-			testName:    "sharded with multi shard destination",
-			sharded:     true,
-			shards:      twoShards,
-			destination: key.DestinationAllShards{},
-			expectedQueryLog: []string{
-				`ResolveDestinations ks [] Destinations:DestinationAllShards()`,
-				`StreamExecuteMulti dummy_query ks.-20: {} ks.20-: {} `,
-			},
-			isDML: true,
-		},
-		{
-			testName:    "sharded with multi shard destination single shard setting",
-			sharded:     true,
-			shards:      twoShards,
-			destination: key.DestinationAllShards{},
-			expectedQueryLog: []string{
-				`ResolveDestinations ks [] Destinations:DestinationAllShards()`,
-			},
-			expectedError:   "Unexpected error, DestinationKeyspaceID mapping to multiple shards: dummy_query, got: DestinationAllShards()",
-			isDML:           true,
-			singleShardOnly: true,
-		},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.testName, func(t *testing.T) {
-			send := &Send{
-				Keyspace: &vindexes.Keyspace{
-					Name:    "ks",
-					Sharded: tc.sharded,
-				},
-				Query:             "dummy_query",
-				TargetDestination: tc.destination,
-				IsDML:             tc.isDML,
-				SingleShardOnly:   tc.singleShardOnly,
-			}
-			vc := &loggingVCursor{shards: tc.shards}
-			_, err := wrapStreamExecute(send, vc, map[string]*querypb.BindVariable{}, false)
-			if tc.expectedError != "" {
-				require.EqualError(t, err, tc.expectedError)
-			} else {
-				require.NoError(t, err)
-			}
-			vc.ExpectLog(t, tc.expectedQueryLog)
-
-			// Failure cases
-			vc = &loggingVCursor{shardErr: errors.New("shard_error")}
-			_, err = wrapStreamExecute(send, vc, map[string]*querypb.BindVariable{}, false)
-			require.EqualError(t, err, "sendStreamExecute: shard_error")
-
-			if !tc.sharded {
-				vc = &loggingVCursor{}
-				_, err = wrapStreamExecute(send, vc, map[string]*querypb.BindVariable{}, false)
-				require.EqualError(t, err, "Keyspace does not have exactly one shard: []")
-			}
-		})
-	}
-}
diff --git a/go/vt/vtgate/planbuilder/testdata/bypass_cases.txt b/go/vt/vtgate/planbuilder/testdata/bypass_cases.txt
index 4fc5d2d023..a3b282848f 100644
--- a/go/vt/vtgate/planbuilder/testdata/bypass_cases.txt
+++ b/go/vt/vtgate/planbuilder/testdata/bypass_cases.txt
@@ -73,22 +73,3 @@
     "SingleShardOnly": false
   }
 }
-
-# insert bypass with sequence: sequences ignored
-"insert into user(nonid) values (2)"
-{
-  "QueryType": "INSERT",
-  "Original": "insert into user(nonid) values (2)",
-  "Instructions": {
-    "OperatorType": "Send",
-    "Variant": "",
-    "Keyspace": {
-      "Name": "main",
-      "Sharded": false
-    },
-    "TargetDestination": "Shard(-80)",
-    "IsDML": true,
-    "Query": "insert into user(nonid) values (2)",
-    "SingleShardOnly": false
-  }
-}
diff --git a/go/vt/vtgate/planbuilder/testdata/select_cases.txt b/go/vt/vtgate/planbuilder/testdata/select_cases.txt
index c4bc3f2532..6de724de00 100644
--- a/go/vt/vtgate/planbuilder/testdata/select_cases.txt
+++ b/go/vt/vtgate/planbuilder/testdata/select_cases.txt
@@ -1365,7 +1365,7 @@
     "OperatorType": "Projection",
     "Variant": "",
     "Columns": [
-      ""
+      "42"
     ],
     "Expressions": [
       "INT64(42)"
@@ -1378,3 +1378,26 @@
     ]
   }
 }
+
+# testing SingleRow Projection with arithmetics
+"select 42+2"
+{
+  "QueryType": "SELECT",
+  "Original": "select 42+2",
+  "Instructions": {
+    "OperatorType": "Projection",
+    "Variant": "",
+    "Columns": [
+      "42 + 2"
+    ],
+    "Expressions": [
+      "INT64(42) + INT64(2)"
+    ],
+    "Inputs": [
+      {
+        "OperatorType": "SingleRow",
+        "Variant": ""
+      }
+    ]
+  }
+}