parser,executor: make `load data` support `user_var` in column list and `set` clause #7707

lysu · 2018-09-14T13:24:39Z

What problem does this PR solve?

from: https://dev.mysql.com/doc/refman/5.7/en/load-data.html

LOAD DATA support

    [(col_name_or_user_var
        [, col_name_or_user_var] ...)]
    [SET col_name={expr | DEFAULT},
        [, col_name={expr | DEFAULT}] ...]

this PR, make TiDB support SET clause and user_var(we are alreay support col_name) which make user have ability to change data during loading data(like exchange column order or call some function to transform), also some type like binary 's import require this feature(just as mysql-doc said).

What is changed and how it works?

make parser support set and @xx in load data
change executor

Check List

Tests

Integration test

Code changes

logic change

Side effects

no

Related changes

Need to update the documentation
Need to be included in the release note

This change is

Remain question

unsupport correlated subquery
it seem ignore/replace is unsupport
enclosed by still unsupport

lysu · 2018-09-14T13:25:52Z

/run-all-tests

lysu · 2018-09-21T11:52:31Z

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

lysu · 2018-09-21T12:48:41Z

@coocood @jackysp PTAL if free thx~

lysu · 2018-09-25T08:26:08Z

PTAL @coocood @jackysp

jackysp · 2018-09-26T03:31:51Z

parser/parser.y

@@ -593,7 +593,10 @@ import (
 	ColumnNameList			"column name list"
 	ColumnList			"column list"
 	ColumnNameListOpt		"column name list opt"
-	ColumnNameListOptWithBrackets 	"column name list opt with brackets"
+	ColumnOrVarListOptWithBrackets 	"column or variable list opt with brackets"
+	ColumnOrVarList         "column or variable list"


Keep aligning.

table/column.go

jackysp · 2018-09-27T05:37:37Z

planner/core/planbuilder.go

@@ -1267,27 +1267,62 @@ func (b *planBuilder) buildSelectPlanOfInsert(insert *ast.InsertStmt, insertPlan
 	return nil
 }

+func (b *planBuilder) buildSetValuesOfLoadData(ld *ast.LoadDataStmt, ldPlan *LoadData, mockTablePlan *LogicalTableDual) error {
+	colNames := make([]string, 0, len(ld.SetList))


where to use it?

jackysp · 2018-09-27T05:38:58Z

planner/core/planbuilder.go

+		exprCols = append(exprCols, exprCol)
+	}
+
+	for i, assign := range ld.SetList {


could we merge this loop with the above one?

jackysp · 2018-09-27T05:42:00Z

planner/core/common_plans.go

+	LinesInfo    *ast.LinesClause
+	IgnoreLines  uint64
+	SetList      []*expression.Assignment
+	tableSchema  *expression.Schema



I think we could remove this empty line now.

jackysp · 2018-09-27T05:42:28Z

parser/parser.y

+ColumnOrVar:
+    UserVariable
+    {
+        $$ = &ast.ColNameOrVar{VariableExpr: $1.(*ast.VariableExpr)}


keep aligning.

jackysp · 2018-09-27T05:42:36Z

parser/parser.y

+    }
+|   ColumnName
+    {
+    	$$ = &ast.ColNameOrVar{ColumnName: $1.(*ast.ColumnName)}


keep aligning.

jackysp · 2018-09-27T06:13:54Z

parser/parser.y

+        }
+
+LoadDataSetOpt:
+    /* EMPTY */


keep aligning

lysu · 2018-09-27T06:34:00Z

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

jackysp · 2018-09-27T06:45:30Z

executor/insert_common.go

+	Columns       []*ast.ColumnName
+	Lists         [][]expression.Expression
+	SetList       []*expression.Assignment
+	ColNameOrVars []*ast.ColNameOrVar


emmm... I think this element is used only for load data. Could we move it out?

server/server_test.go

jackysp · 2018-09-27T08:37:01Z

server/server_test.go

+		dbt.Check(err, IsNil)
+		dbt.Check(*aa, DeepEquals, 0)
+		dbt.Check(*bb, DeepEquals, 0)
+		//dbt.Check(cc, DeepEquals, ([]uint8)(nil)) FIXME: stranger


should it be fixed?

lysu · 2018-09-27T13:55:19Z

@coocood ColNameOrVar has be removed, PTAL thx~

jackysp · 2018-10-08T06:17:32Z

executor/builder.go

+		}
+		colsBind, err := table.FindCols(tableCols, columns, e.Table.Meta().PKIsHandle)
+		if err != nil {
+			return nil, 0, errors.Errorf("LOAD DATA SET %s: %s", e.Table.Meta().Name.O, err)


what kind of error will MySQL returned?

jackysp · 2018-10-08T08:57:32Z

planner/core/planbuilder.go

+			return errors.Trace(err)
+		}
+		if exprCol == nil {
+			return errors.Errorf("Can't find column %s", assign.Column)


It's better to return the same error as MySQL does.

jackysp

rest LGTM

lysu · 2018-10-10T06:12:22Z

PTAL @jackysp

jackysp · 2018-10-10T07:30:58Z

executor/load_data.go

+		}
+	}
+
+	mRow := chunk.MutRowFromDatums(row).ToRow()


Could we skip this if the SetList is nil?

lysu · 2018-10-12T11:15:11Z

PTAL @jackysp @tiancaiamao

jackysp

LGTM

tiancaiamao · 2018-10-15T12:12:54Z

ast/dml.go

@@ -685,11 +686,20 @@ func (n *LoadDataStmt) Accept(v Visitor) (Node, bool) {
 		n.Table = node.(*TableName)
 	}
 	for i, val := range n.Columns {
-		node, ok := val.Accept(v)
+		if colExpr, isCol := val.(*ColumnNameExpr); isCol {


How about this

switch val.(type) { case *ColumnNameExpr: case *VariableExpr: }

tiancaiamao · 2018-10-15T12:14:59Z

ast/dml.go

@@ -664,10 +664,11 @@ type LoadDataStmt struct {
 	IsLocal     bool
 	Path        string
 	Table       *TableName
-	Columns     []*ColumnName
+	Columns     []ExprNode


Rename it to ColumnOrVars ?

tiancaiamao · 2018-10-15T12:19:47Z

executor/builder.go

+			if colName, isCol := colOrVar.(*ast.ColumnNameExpr); isCol {
+				columns = append(columns, colName.Name.Name.O)
+			} else {
+				columns = append(columns, "")


table.FindCols(tableCols, "", ...)

What's the purpose of this line ?

had added some comments to this line in code :)

tiancaiamao · 2018-10-15T12:24:45Z

executor/builder.go

@@ -559,6 +559,55 @@ func (b *executorBuilder) buildInsert(v *plannercore.Insert) Executor {
 	return insert
 }

+func getColumnInfo4LoadData(e *InsertValues, tableCols []*table.Column, colOrVars []ast.ExprNode) ([]*table.Column, int, error) {


Add comment for the returned int value

tiancaiamao · 2018-10-15T13:18:22Z

server/server_test.go

 		t.Assert(outB, Equals, "23:59:59")
 	})
 }

+func runTestLoadDataWithSet(c *C, server *Server) {
+	path := "/tmp/load_data_test2.csv"


Consider this API:
https://godoc.org/io/ioutil#TempFile

tiancaiamao · 2018-10-15T13:21:45Z

server/server_test.go

+			bb *int
+			cc []uint8
+		)
+		rows := dbt.mustQuery("select a, b, c from t")


This kind of code could be simplified with the util/testkit utilities.

it seems server_test.go never use testkit? 😹 because testkit is kv.Storage based, and integration test need sql.DB?

lysu · 2018-10-19T02:38:04Z

executor/load_data.go

+		mRow := chunk.MutRowFromDatums(row).ToRow()
+		for _, setExpr := range e.SetList {
+			colInfo := e.colInfo[i]
+			v, err := setExpr.Expr.Eval(mRow)


here need wait #7935 , can reuse one buffer instead renew row by row.

zz-jason · 2018-11-28T14:07:39Z

any update?

lysu added status/WIP type/compatibility component/mysql-protocol labels Sep 14, 2018

lysu changed the title ~~parser,executor: make load data support set clause and user_var in column list~~ parser,executor: make load data support user_var in column list and set clause Sep 14, 2018

lysu removed the status/WIP label Sep 17, 2018

lysu added the status/all tests passed label Sep 21, 2018

lysu assigned jackysp Sep 25, 2018

jackysp reviewed Sep 26, 2018

View reviewed changes