Repository: vitessio/vitess
Pull request #8875 (2021-09-24 05:55:52)

added checks for next val misplaced statements and validate the table is sequence table
<!--
  Thank you for your contribution to the Vitess project.
  How to contribute: https://vitess.io/docs/contributing/
  Please first make sure there is an open Issue to discuss the feature/fix suggested in this PR.
  If this is a new feature, please mark the Issue as "RFC".
 -->

<!-- if this PR is Work in Progress please create it as a Draft Pull Request -->

## Description
<!-- A few sentences describing the overall goals of the pull request's commits. -->
<!-- If this is a bug fix and you think the fix should be backported, please write so. -->

This PR fails the wrong usage pf next val statements.

## Related Issue(s)
<!-- List related issues and pull requests. If this PR fixes an issue, please add it using Fixes #????  -->

- #7280 

## Checklist
- [X] Tests were added or are not required
- [X] Documentation was added or is not required

## Deployment Notes
<!-- Notes regarding deployment of the contained body of work. These should note any db migrations, etc. -->


Patch:
diff --git a/go/vt/vtgate/planbuilder/abstract/queryprojection.go b/go/vt/vtgate/planbuilder/abstract/queryprojection.go
index ebd731b71b..32ef786d25 100644
--- a/go/vt/vtgate/planbuilder/abstract/queryprojection.go
+++ b/go/vt/vtgate/planbuilder/abstract/queryprojection.go
@@ -64,14 +64,11 @@ type (
 // GetExpr returns the underlying sqlparser.Expr of our SelectExpr
 func (s SelectExpr) GetExpr() (sqlparser.Expr, error) {
 	switch sel := s.Col.(type) {
-	case *sqlparser.StarExpr:
-		return nil, vterrors.New(vtrpcpb.Code_INTERNAL, "starExpr does not have expr")
 	case *sqlparser.AliasedExpr:
 		return sel.Expr, nil
-	case *sqlparser.Nextval:
-		return sel.Expr, nil
+	default:
+		return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "[BUG] %T does not have expr", s.Col)
 	}
-	return nil, nil
 }
 
 // GetAliasedExpr returns the SelectExpr as a *sqlparser.AliasedExpr if its type allows it,
@@ -156,7 +153,7 @@ func (qp *QueryProjection) addSelectExpressions(sel *sqlparser.Select) error {
 			}
 			qp.SelectExprs = append(qp.SelectExprs, col)
 		default:
-			return semantics.Gen4NotSupportedF("%T in select list", selExp)
+			return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "[BUG] %T in select list", selExp)
 		}
 	}
 	return nil
diff --git a/go/vt/vtgate/semantics/analyzer.go b/go/vt/vtgate/semantics/analyzer.go
index a72e935bf8..c035debb5b 100644
--- a/go/vt/vtgate/semantics/analyzer.go
+++ b/go/vt/vtgate/semantics/analyzer.go
@@ -21,6 +21,8 @@ import (
 	"runtime/debug"
 	"strings"
 
+	"vitess.io/vitess/go/vt/vtgate/vindexes"
+
 	querypb "vitess.io/vitess/go/vt/proto/query"
 	vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
 	"vitess.io/vitess/go/vt/sqlparser"
@@ -125,11 +127,11 @@ func (a *analyzer) analyzeDown(cursor *sqlparser.Cursor) bool {
 	}
 
 	if !a.hasRewritten {
-		if err := checkForInvalidConstructs(cursor); err != nil {
+		if err := a.scoper.down(cursor); err != nil {
 			a.setError(err)
 			return true
 		}
-		if err := a.scoper.down(cursor); err != nil {
+		if err := a.checkForInvalidConstructs(cursor); err != nil {
 			a.setError(err)
 			return true
 		}
@@ -263,15 +265,41 @@ func (a *analyzer) analyze(statement sqlparser.Statement) error {
 	return a.err
 }
 
-func checkForInvalidConstructs(cursor *sqlparser.Cursor) error {
+func (a *analyzer) checkForInvalidConstructs(cursor *sqlparser.Cursor) error {
 	switch node := cursor.Node().(type) {
 	case *sqlparser.Select:
-		if node.Into == nil {
+		if a.scoper.currentScope() == nil {
 			return nil
 		}
-		if _, isRootNode := cursor.Parent().(*sqlparser.RootNode); !isRootNode {
+		errMsg := "INTO"
+		nextVal := false
+		if len(node.SelectExprs) == 1 {
+			if _, isNextVal := node.SelectExprs[0].(*sqlparser.Nextval); isNextVal {
+				nextVal = true
+				errMsg = "NEXT"
+			}
+		}
+		if !nextVal && node.Into == nil {
+			return nil
+		}
+		if a.scoper.currentScope().parent != nil {
+			return vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.CantUseOptionHere, "Incorrect usage/placement of '%s'", errMsg)
+		}
+	case *sqlparser.Nextval:
+		currScope := a.scoper.currentScope()
+		if currScope.parent != nil {
 			return vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.CantUseOptionHere, "Incorrect usage/placement of 'INTO'")
 		}
+		if len(currScope.tables) != 1 {
+			return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "[BUG] Next statement should not contain multiple tables")
+		}
+		vindexTbl := currScope.tables[0].GetVindexTable()
+		if vindexTbl == nil {
+			return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Table information is not provided in vschema")
+		}
+		if vindexTbl.Type != vindexes.TypeSequence {
+			return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "NEXT used on a non-sequence table")
+		}
 	case *sqlparser.JoinTableExpr:
 		if node.Condition != nil && node.Condition.Using != nil {
 			return vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: join with USING(column_list) clause for complex queries")


Test patch:
diff --git a/go/vt/sqlparser/parse_test.go b/go/vt/sqlparser/parse_test.go
index af97b7cafd..314b42af42 100644
--- a/go/vt/sqlparser/parse_test.go
+++ b/go/vt/sqlparser/parse_test.go
@@ -2131,6 +2131,18 @@ func TestInvalid(t *testing.T) {
 	}, {
 		input: "select a from x order by y union select a from c",
 		err:   "syntax error",
+	}, {
+		input: "select next 2 values from seq union select next value from seq",
+		err:   "syntax error at position 36 near 'union'",
+	}, {
+		input: "select next 2 values from user where id = 1",
+		err:   "syntax error at position 37 near 'where'",
+	}, {
+		input: "select next 2 values from seq, seq",
+		err:   "syntax error at position 31",
+	}, {
+		input: "select 1, next value from seq",
+		err:   "syntax error",
 	}}
 
 	for _, tcase := range invalidSQL {
diff --git a/go/vt/vtgate/planbuilder/abstract/queryprojection_test.go b/go/vt/vtgate/planbuilder/abstract/queryprojection_test.go
index 0977ff854e..a36a62d440 100644
--- a/go/vt/vtgate/planbuilder/abstract/queryprojection_test.go
+++ b/go/vt/vtgate/planbuilder/abstract/queryprojection_test.go
@@ -38,10 +38,6 @@ func TestQP(t *testing.T) {
 		{
 			sql: "select * from user",
 		},
-		{
-			sql:    "select next value from user_seq",
-			expErr: "gen4 does not yet support: *sqlparser.Nextval in select list",
-		},
 		{
 			sql:    "select 1, count(1) from user",
 			expErr: "In aggregated query without GROUP BY, expression of SELECT list contains nonaggregated column '1'; this is incompatible with sql_mode=only_full_group_by",
diff --git a/go/vt/vtgate/planbuilder/testdata/from_cases.txt b/go/vt/vtgate/planbuilder/testdata/from_cases.txt
index ee7e268d44..7943a1e0fa 100644
--- a/go/vt/vtgate/planbuilder/testdata/from_cases.txt
+++ b/go/vt/vtgate/planbuilder/testdata/from_cases.txt
@@ -55,6 +55,125 @@ Gen4 plan same as above
 }
 Gen4 plan same as above
 
+# select next from non-sequence table
+"select next value from user"
+"NEXT used on a non-sequence table"
+Gen4 plan same as above
+
+# select next in derived table
+"select 1 from (select next value from seq) t"
+{
+  "QueryType": "SELECT",
+  "Original": "select 1 from (select next value from seq) t",
+  "Instructions": {
+    "OperatorType": "Route",
+    "Variant": "SelectNext",
+    "Keyspace": {
+      "Name": "main",
+      "Sharded": false
+    },
+    "FieldQuery": "select 1 from (select next 1 values from seq where 1 != 1) as t where 1 != 1",
+    "Query": "select 1 from (select next 1 values from seq) as t",
+    "Table": "seq"
+  }
+}
+Gen4 error: Incorrect usage/placement of 'NEXT'
+
+# select next in derived table
+"select * from (select next value from seq) t"
+{
+  "QueryType": "SELECT",
+  "Original": "select * from (select next value from seq) t",
+  "Instructions": {
+    "OperatorType": "Route",
+    "Variant": "SelectNext",
+    "Keyspace": {
+      "Name": "main",
+      "Sharded": false
+    },
+    "FieldQuery": "select * from (select next 1 values from seq where 1 != 1) as t where 1 != 1",
+    "Query": "select * from (select next 1 values from seq) as t",
+    "Table": "seq"
+  }
+}
+Gen4 error: Incorrect usage/placement of 'NEXT'
+
+# select next in subquery
+"select 1 from user where id in (select next value from seq)"
+{
+  "QueryType": "SELECT",
+  "Original": "select 1 from user where id in (select next value from seq)",
+  "Instructions": {
+    "OperatorType": "Subquery",
+    "Variant": "PulloutIn",
+    "Inputs": [
+      {
+        "OperatorType": "Route",
+        "Variant": "SelectNext",
+        "Keyspace": {
+          "Name": "main",
+          "Sharded": false
+        },
+        "FieldQuery": "select next 1 values from seq where 1 != 1",
+        "Query": "select next 1 values from seq",
+        "Table": "seq"
+      },
+      {
+        "OperatorType": "Route",
+        "Variant": "SelectIN",
+        "Keyspace": {
+          "Name": "user",
+          "Sharded": true
+        },
+        "FieldQuery": "select 1 from `user` where 1 != 1",
+        "Query": "select 1 from `user` where :__sq_has_values1 = 1 and id in ::__vals",
+        "Table": "`user`",
+        "Values": [
+          "::__sq1"
+        ],
+        "Vindex": "user_index"
+      }
+    ]
+  }
+}
+Gen4 error: Incorrect usage/placement of 'NEXT'
+
+# select next in projection
+"select (select next value from seq) from user"
+{
+  "QueryType": "SELECT",
+  "Original": "select (select next value from seq) from user",
+  "Instructions": {
+    "OperatorType": "Subquery",
+    "Variant": "PulloutValue",
+    "Inputs": [
+      {
+        "OperatorType": "Route",
+        "Variant": "SelectNext",
+        "Keyspace": {
+          "Name": "main",
+          "Sharded": false
+        },
+        "FieldQuery": "select next 1 values from seq where 1 != 1",
+        "Query": "select next 1 values from seq",
+        "Table": "seq"
+      },
+      {
+        "OperatorType": "Route",
+        "Variant": "SelectScatter",
+        "Keyspace": {
+          "Name": "user",
+          "Sharded": true
+        },
+        "FieldQuery": "select :__sq1 from `user` where 1 != 1",
+        "Query": "select :__sq1 from `user`",
+        "Table": "`user`"
+      }
+    ]
+  }
+}
+Gen4 error: Incorrect usage/placement of 'NEXT'
+
 # Select from reference
 "select * from ref"
 {
diff --git a/go/vt/vtgate/planbuilder/testdata/select_cases.txt b/go/vt/vtgate/planbuilder/testdata/select_cases.txt
index 5e3943dfbc..49b8471fdd 100644
--- a/go/vt/vtgate/planbuilder/testdata/select_cases.txt
+++ b/go/vt/vtgate/planbuilder/testdata/select_cases.txt
@@ -568,10 +568,6 @@ Gen4 plan same as above
 }
 Gen4 plan same as above
 
-# nextval for simple route
-"select next value from user"
-"NEXT used on a non-sequence table"
-
 # last_insert_id for unsharded route
 "select last_insert_id() as x from main.unsharded"
 {