*: remove resolver.go #4988

XuHuaiyu · 2017-11-02T08:42:42Z

refer to #4787
should be merged with https://github.com/pingcap/tidb-test/pull/398
This commit removes resolver.go completely.
Now TiDB resolves table name in preprocessor.go, and resolves column names when building logical plan.

Resolving col name when building logical plan will raise a problem like:

create table t(a int);
prepare stmt from 'select b from t'; // This sql should have caused an error, I'll fix it in next PR.

Conflicts: executor/prepared.go executor/prepared_test.go plan/physical_plan_test.go

XuHuaiyu · 2017-11-02T08:42:53Z

/run-all-test

hanfei1991 · 2017-11-02T09:17:33Z

plan/resolver_test.go

-}
-
-var resolverTests = []resolverTestCase{
-	{"select c1 from t1", true, ""},


Some cases need to move to logicalPlanBuilder TestPlanBuilder

hanfei1991 · 2017-11-02T09:18:53Z

plan/preprocess.go

+}
+
+func (p *preprocessor) handleUnionSelectList(u *ast.UnionSelectList) {
+	firstSelFields := u.Selects[0].GetResultFields()


Why do we need to change result fields. It seems we don't depend on it any more.

hanfei1991 · 2017-11-02T09:19:45Z

plan/preprocess.go

@@ -499,6 +523,19 @@ func (p *preprocessor) handleTableName(tn *ast.TableName) {
 		// Skip resolving the table to avoid error.
 		return
 	}
+	if p.inDeleteTableList {


it is only for delete statement. can we move this to buildDelete function ?

Conflicts: plan/resolver.go

XuHuaiyu · 2017-11-06T14:09:44Z

CI failed since #4613 relies on GetResultField of *ast.Join ....
I'll fix it soon.

XuHuaiyu · 2017-11-07T04:54:15Z

/run-all-tests

�ý,�ý`�ý.:q1

hanfei1991 · 2017-11-07T05:59:06Z

new_session_test.go

 	c.Assert(err, IsNil)
-	c.Assert(fields, HasLen, 1)
+	// We do not create ResultFields anymore when preprocessing.


So can we remove this assert ?

hanfei1991 · 2017-11-07T06:00:52Z

plan/dag_plan_test.go

@@ -1040,16 +1048,17 @@ func (s *testPlanSuite) TestRefine(c *C) {
 		},
 		{
 			sql:  `select a from t where c = 'hanfei'`,
-			best: "TableReader(Table(t))->Sel([eq(cast(test.t.c), cast(hanfei))])->Projection",
+			best: "IndexReader(Index(t.c_d_e)[[0,0]])->Projection",


Why is this test changed ?

use test set sc.IgnoreTruncate to true,
SelectStmt reset this when executing, we only compile statement and reuse the sc.IgnoreTruncate set be UseStmt here, so this case changed.

hanfei1991 · 2017-11-07T06:02:19Z

plan/logical_plan_builder.go

@@ -902,7 +909,11 @@ func (a *havingAndOrderbyExprResolver) Leave(n ast.Node) (node ast.Node, ok bool
 					return n, true
 				}
 			}
-			a.err = errors.Errorf("Unknown Column %s", v.Name.Name.L)
+			cn := v.Name.Name.L


It seems this logic will be used in many places. Can we make it a function ?

I didn't really find another place use this logic totally the same.
May keep it?

hanfei1991 · 2017-11-07T06:03:54Z

plan/optimizer.go

@@ -173,13 +173,7 @@ func existsCartesianProduct(p LogicalPlan) bool {
 // The statement must be prepared before it can be passed to optimize function.
 // We pass InfoSchema instead of getting from Context in case it is changed after resolving name.
 func PrepareStmt(is infoschema.InfoSchema, ctx context.Context, node ast.Node) error {


Do we still need this function ??

@XuHuaiyu how about removing it ?

XuHuaiyu · 2017-11-07T08:00:45Z

PTAL @hanfei1991

XuHuaiyu · 2017-11-07T08:06:15Z

/run-all-tests

XuHuaiyu · 2017-11-07T10:45:57Z

CI failed since some error messages changed.
I'll try to fix them soon.

�ý,�ý.�ý`:q1

zimulala · 2017-11-08T06:44:02Z

plan/expression_rewriter.go

@@ -28,6 +28,7 @@ import (
 	"github.com/pingcap/tidb/sessionctx/variable"
 	"github.com/pingcap/tidb/sessionctx/varsutil"
 	"github.com/pingcap/tidb/types"
+	"strconv"


Move this line up.

zimulala · 2017-11-08T06:48:16Z

plan/logical_plan_builder.go

-				return g.fields[index].Expr, true
+				ret := g.fields[index].Expr
+				if _, ok := ret.(*ast.AggregateFuncExpr); ok {
+					err = ErrIllegalReference.Gen("Reference '%s' not supported (reference to group function)", v.Name.OrigColName())


I think you can update the content of ErrIllegalReference directly.

zimulala · 2017-11-08T06:53:34Z

plan/logical_plan_builder.go

+				if _, ok := ret.(*ast.AggregateFuncExpr); ok {
+					err = ErrIllegalReference.Gen("Reference '%s' not supported (reference to group function)", v.Name.OrigColName())
+				} else {
+					return g.fields[index].Expr, true


s/g.fields[index].Expr/ret

zimulala · 2017-11-08T07:01:45Z

plan/logical_plan_builder.go

+			foundMatch := false
+			for _, v := range tableList {
+				if dn := v.Schema.L; dn != "" {
+					dbName = dn


If the first database name isn't empty and isn't the current database, and the next database name is empty, then the next database isn't the current database. Is there something wrong?

zimulala · 2017-11-08T07:10:32Z

plan/preprocess.go

+	p.tables = append(p.tables, ts)
+}
+
+func getInnerFromParentheses(expr ast.ExprNode) ast.ExprNode {


Do we use it?

zimulala · 2017-11-08T07:18:13Z

plan/preprocess.go

+		name := f.ColumnAsName.L
+		if name == "" {
+			name = f.Column.Name.L
+		}


This name doesn't use. These codes are duplicated with follows.

hanfei1991 · 2017-11-08T07:55:06Z

plan/logical_plan_builder.go

@@ -1004,7 +1013,12 @@ func (g *gbyResolver) Leave(inNode ast.Node) (ast.Node, bool) {
 				return inNode, true
 			}
 			if index != -1 {
-				return g.fields[index].Expr, true
+				ret := g.fields[index].Expr
+				if _, ok := ret.(*ast.AggregateFuncExpr); ok {


It maybe an expression that contains a aggregate function.

hanfei1991 · 2017-11-08T07:55:29Z

plan/logical_plan_builder.go

@@ -1014,24 +1028,28 @@ func (g *gbyResolver) Leave(inNode ast.Node) (ast.Node, bool) {
 			g.err = errors.Errorf("Unknown column '%d' in 'group statement'", v.N)
 			return inNode, false
 		}
-		return g.fields[v.N-1].Expr, true
+		ret := g.fields[v.N-1].Expr
+		if _, ok := ret.(*ast.AggregateFuncExpr); ok {


hanfei1991 · 2017-11-08T07:58:37Z

plan/logical_plan_builder.go

@@ -1416,6 +1440,17 @@ func (b *planBuilder) buildSelect(sel *ast.SelectStmt) LogicalPlan {
 	)
 	if sel.From != nil {
 		p = b.buildResultSetNode(sel.From.TableRefs)
+		// duplicate column name in one table is not allowed.


We should do this check in buildResultSetNode.

hanfei1991 · 2017-11-08T08:01:12Z

plan/logical_plan_builder.go

+					break
+				}
+			}
+			if !foundMatch {


What's the behaviour of delete a from (select * from t)a, t;

hanfei1991 · 2017-11-08T08:03:01Z

plan/preprocess.go

@@ -105,8 +116,17 @@ func (p *preprocessor) Leave(in ast.Node) (out ast.Node, ok bool) {
 		if !valid {
 			p.err = ErrUnknownExplainFormat.GenByArgs(x.Format)
 		}
+	case *ast.SelectStmt:
+		x.SetResultFields(p.fieldList)


Why do we still hold result fields ? can we remove this ?

XuHuaiyu · 2017-11-08T10:24:10Z

/run-common-test tidb-test=pr4988
/run-integration-common-tests tidb-test=pr4988

XuHuaiyu · 2017-11-08T14:00:52Z

/run-all-tests tidb-test=pr/398

XuHuaiyu · 2017-11-08T14:31:36Z

PTAL @hanfei1991 @zimulala @zz-jason

zz-jason · 2017-11-08T14:37:20Z

executor/prepared_test.go

@@ -89,7 +89,7 @@ func (s *testSuite) TestPrepared(c *C) {

 		// There should be schema changed error.
 		_, err = tk.Se.ExecutePreparedStmt(stmtId, 1)
-		c.Assert(plan.ErrSchemaChanged.Equal(err), IsTrue)
+		c.Assert(err, NotNil)


check the error message ?

zz-jason · 2017-11-08T14:39:53Z

plan/optimizer.go

 	CodeNoDB                 terror.ErrCode = mysql.ErrNoDB
 	CodeUnknownExplainFormat terror.ErrCode = mysql.ErrUnknownExplainFormat
+	CodeWrongGroupField                     = mysql.ErrWrongGroupField


how about add terror.ErrCode to be compatible with other error code definitions ?

zz-jason · 2017-11-08T14:40:22Z

plan/optimizer.go

@@ -173,13 +173,7 @@ func existsCartesianProduct(p LogicalPlan) bool {
 // The statement must be prepared before it can be passed to optimize function.
 // We pass InfoSchema instead of getting from Context in case it is changed after resolving name.
 func PrepareStmt(is infoschema.InfoSchema, ctx context.Context, node ast.Node) error {


@XuHuaiyu how about removing it ?

zz-jason · 2017-11-08T14:43:17Z

plan/planbuilder.go

+type clauseCode int
+
+const (
+	unknowClause  clauseCode = 0


unknowClause clauseCode = iota

how about let all clause code have a common prefix, like "clauseCodeXXX" ?

For my part, the current variable names may be better.

zz-jason · 2017-11-08T14:51:49Z

plan/preprocess.go

@@ -561,3 +571,38 @@ func (p *preprocessor) resolveAlterTableStmt(node *ast.AlterTableStmt) {
 		}
 	}
 }
+
+// handleTableSource checks name duplication


... checks origin table source duplication ... ?

zz-jason · 2017-11-08T14:55:42Z

plan/preprocess.go

@@ -48,6 +48,8 @@ type preprocessor struct {
 	/* For Select Statement. */
 	// table map to lookup and check table name conflict.
 	tableMap map[string]int
+	// table map to lookup and check derived-table(subselect) name conflict.
+	derivedTableMap map[string]int


Is the int map values meaningless ? If so, how about derivedTableSet map[string]struct{} ?

The int value is useful.

zz-jason · 2017-11-08T14:59:08Z

plan/typeinfer_test.go

-		err = plan.ResolveName(stmt, is, ctx)
-		c.Assert(err, IsNil, comment)
+		err = plan.Preprocess(ctx, stmt, is, false)
+		c.Assert(err, IsNil)


add comment back ?

;�ý`q�kb��:q!

�ý,�ý`�ý.:q!

…lver :Q!

zimulala · 2017-11-09T04:37:02Z

LGTM

hanfei1991 · 2017-11-08T13:43:31Z

executor/prepared_test.go

@@ -89,7 +89,7 @@ func (s *testSuite) TestPrepared(c *C) {

 		// There should be schema changed error.
 		_, err = tk.Se.ExecutePreparedStmt(stmtId, 1)
-		c.Assert(plan.ErrSchemaChanged.Equal(err), IsTrue)
+		c.Assert(err, NotNil)


Why change this

hanfei1991 · 2017-11-08T13:44:33Z

new_session_test.go

@@ -1119,6 +1118,11 @@ func (s *testSessionSuite) TestDelete(c *C) {
 	tk.MustExec("insert into t (F1) values ('1'), ('2');")
 	tk.MustExec("delete test1.t from test1.t inner join test.t where test1.t.F1 > test.t.F1")
 	tk1.MustQuery("select * from t;").Check(testkit.Rows("1"))
+
+	_, err := tk.Exec("delete a from (select * from t ) as a, t")


This test should be moved to logical plan test.

hanfei1991 · 2017-11-09T06:28:26Z

plan/plans.go

@@ -119,9 +119,7 @@ func (e *Execute) optimizePreparedPlan(ctx context.Context, is infoschema.InfoSc
 		vars.PreparedParams[i] = val
 	}
 	if prepared.SchemaVersion != is.SchemaMetaVersion() {
-		// If the schema version has changed we need to prepare it again,


Why remove the comments ?

hanfei1991 · 2017-11-09T06:30:44Z

plan/logical_plan_builder.go

@@ -151,6 +151,17 @@ func (b *planBuilder) buildResultSetNode(node ast.ResultSetNode) LogicalPlan {
 				col.DBName = model.NewCIStr("")
 			}
 		}
+		// duplicate column name in one table is not allowed.


duplicate -> Duplicate

hanfei1991 · 2017-11-09T06:34:06Z

plan/logical_plan_builder.go

 				colName := &ast.ColumnNameExpr{
 					Name: &ast.ColumnName{
 						Schema: col.DBName,
 						Table:  col.TblName,
 						Name:   col.ColName,
 					}}
+				ast.SetFlag(colName)


Why do we need to set flag for column name ? I grep the whole code. It seems there is the only place that sets flag

hanfei1991 · 2017-11-09T06:40:54Z

plan/preprocess.go

+			name = p.tableUniqueName(tableName.Schema, tableName.Name)
+		}
+		if _, ok := p.tableMap[name]; ok {
+			p.err = errors.Errorf("duplicated table/alias name %s", name)


Duplicated table name would be checked in logical plan builder ?

:q!

XuHuaiyu · 2017-11-09T10:59:26Z

PTAL @hanfei1991

XuHuaiyu · 2017-11-09T11:09:02Z

/run-all-tests tidb-test=pr/398

XuHuaiyu · 2017-11-09T11:32:06Z

/run-common-test tidb-test=pr/398
/run-integration-common-tests tidb-test=pr/398

�ý,�ý`�ý.:q!

…lver :1�kbQ1 �ý`q!�kb��:q!

hanfei1991 · 2017-11-10T05:46:53Z

LGTM

zz-jason

LGTM

XuHuaiyu added 2 commits November 2, 2017 16:33

*: remove resolver.go

3a72dfe

Merge branch 'master' of https://github.com/pingcap/tidb into resolver

aab8374

Conflicts: executor/prepared.go executor/prepared_test.go plan/physical_plan_test.go

hanfei1991 reviewed Nov 2, 2017

View reviewed changes

XuHuaiyu added 2 commits November 6, 2017 21:02

address comment

6c74dce

Merge branch 'master' of https://github.com/pingcap/tidb into resolver

a4b975e

Conflicts: plan/resolver.go

XuHuaiyu added 2 commits November 7, 2017 12:57

fix ci

e358269

Merge branch 'master' of https://github.com/pingcap/tidb into resolver

cf24333

�ý,�ý`�ý.:q1

XuHuaiyu force-pushed the resolver branch from 75ab995 to cf24333 Compare November 7, 2017 04:57

remove useless code

f27cde5

hanfei1991 reviewed Nov 7, 2017

View reviewed changes

fix ci

5aa6020

XuHuaiyu added this to the 1.1 milestone Nov 7, 2017

XuHuaiyu added 3 commits November 8, 2017 13:13

correct some error messages

c58eb83

Merge branch 'master' of https://github.com/pingcap/tidb into resolver

d793b29

�ý,�ý.�ý`:q1

fix ci

db80020

XuHuaiyu force-pushed the resolver branch from 5ab8ef2 to db80020 Compare November 8, 2017 05:32

zimulala reviewed Nov 8, 2017

View reviewed changes

XuHuaiyu added 2 commits November 8, 2017 15:30

address comment

c01529a

address comment

ac25b72

hanfei1991 reviewed Nov 8, 2017

View reviewed changes

XuHuaiyu added 2 commits November 8, 2017 18:15

address comment

7ebffc7

remove useless code

1a84261

XuHuaiyu force-pushed the resolver branch from 4ca6edd to 1a84261 Compare November 8, 2017 10:19

Merge branch 'master' into resolver

7c87fe4

zz-jason reviewed Nov 8, 2017

View reviewed changes

XuHuaiyu added 4 commits November 9, 2017 10:27

Merge branch 'master' of https://github.com/pingcap/tidb into resolver

9c93397

;�ý`q�kb��:q!

address comment

827c1e5

Merge branch 'master' of https://github.com/pingcap/tidb into resolver

6ce935e

�ý,�ý`�ý.:q!

Merge branch 'resolver' of https://github.com/XuHuaiyu/tidb into reso…

5be3106

…lver :Q!

zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 9, 2017

add some comments

3ab3709

hanfei1991 reviewed Nov 9, 2017

View reviewed changes

XuHuaiyu added 3 commits November 9, 2017 18:56

address comment

4f4e750

Merge branch 'master' of https://github.com/pingcap/tidb into resolver

101e53d

:q!

remove useless code

569295b

XuHuaiyu added 4 commits November 9, 2017 20:43

Merge branch 'master' into resolver

9c25d97

Merge branch 'master' of https://github.com/pingcap/tidb into resolver

62de660

�ý,�ý`�ý.:q!

remove useless code

88df13c

Merge branch 'resolver' of https://github.com/XuHuaiyu/tidb into reso…

461f103

…lver :1�kbQ1 �ý`q!�kb��:q!

hanfei1991 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 10, 2017

zz-jason approved these changes Nov 10, 2017

View reviewed changes

Merge branch 'master' into resolver

35f7346

coocood merged commit 1521258 into pingcap:master Nov 10, 2017

XuHuaiyu deleted the resolver branch November 10, 2017 07:20

kennytm mentioned this pull request Jan 22, 2020

change all dot import, according to staticcheck pingcap/parser#730

Merged

*: remove resolver.go #4988

*: remove resolver.go #4988

Conversation

XuHuaiyu commented Nov 2, 2017 • edited

XuHuaiyu commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuHuaiyu commented Nov 6, 2017

XuHuaiyu commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuHuaiyu commented Nov 7, 2017

XuHuaiyu commented Nov 7, 2017

XuHuaiyu commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuHuaiyu commented Nov 8, 2017

XuHuaiyu commented Nov 8, 2017

XuHuaiyu commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason Nov 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuHuaiyu commented Nov 9, 2017

XuHuaiyu commented Nov 9, 2017

XuHuaiyu commented Nov 9, 2017

hanfei1991 commented Nov 10, 2017

zz-jason left a comment

Choose a reason for hiding this comment

XuHuaiyu commented Nov 2, 2017 •

edited

zz-jason Nov 8, 2017 •

edited