Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: Support batch load data #2394

Merged
merged 7 commits into from
Jan 5, 2017
Merged

*: Support batch load data #2394

merged 7 commits into from
Jan 5, 2017

Conversation

zimulala
Copy link
Contributor

@zimulala zimulala commented Jan 4, 2017

Fix #1771

@@ -540,7 +552,8 @@ func (e *LoadData) Close() error {

// InsertValues is the data to insert.
type InsertValues struct {
currRow int
currRow int64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use uint64 is safer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think don't it's necessary.

@@ -630,8 +630,9 @@ func (s *testSuite) TestLoadData(c *C) {
deleteSQL := "delete from load_data_test"
selectSQL := "select * from load_data_test;"
// data1 = nil, data2 = nil, fields and lines is default
_, err = ld.InsertData(nil, nil)
_, reachLimit, err := ld.InsertData(nil, nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add test for reach limit is true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is in the file of server/tidb_test.go.


txn := loadDataInfo.Ctx.Txn()
if err != nil {
if err1 := txn.Rollback(); err1 != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use err directly?

break
}
loadDataInfo.Ctx.NewTxn()
shouldCommit = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unuseful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which one?

@@ -411,12 +417,18 @@ func (e *LoadDataInfo) InsertData(prevData, curData []byte) ([]byte, error) {
cols = escapeCols(rawCols)
e.insertData(cols)
e.insertVal.currRow++
if e.insertVal.batchRows != 0 && e.insertVal.currRow%e.insertVal.batchRows == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think use mod operator is not efficiently. You can let currRow = 0 every time it reaches batchRows, and then check currRow >= batchRows.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The currRow is useful to other place.

@@ -562,15 +550,38 @@ func (cc *clientConn) handleLoadData(loadDataInfo *executor.LoadDataInfo) error
break
}
}
prevData, err = loadDataInfo.InsertData(prevData, curData)
for {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This for loop can be extracted to a function.

@@ -562,15 +550,38 @@ func (cc *clientConn) handleLoadData(loadDataInfo *executor.LoadDataInfo) error
break
}
}
prevData, err = loadDataInfo.InsertData(prevData, curData)
for {
prevData, shouldCommit, err = loadDataInfo.InsertData(prevData, curData)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldCommit or reachLimit? We better keep it consistent.

@@ -603,6 +614,7 @@ func (cc *clientConn) handleQuery(sql string) (err error) {
} else {
loadDataInfo := cc.ctx.Value(executor.LoadDataVarKey)
if loadDataInfo != nil {
defer cc.ctx.SetValue(executor.LoadDataVarKey, nil)
if err = cc.handleLoadData(loadDataInfo.(*executor.LoadDataInfo)); err != nil {
return errors.Trace(err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about rollback or commit here, so we can return instead of break in handlLoadData

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK to use break. And I want to put this logic in the same function.

:
@@ -0,0 +1,834 @@
// Copyright 2016 PingCAP, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this file name is ":" ?

return nil, errors.Trace(err)
}
loadDataInfo.Ctx.NewTxn()
reachLimit = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this line.

@hanfei1991
Copy link
Member

LGTM

if err = loadDataInfo.Ctx.Txn().Commit(); err != nil {
return nil, errors.Trace(err)
}
loadDataInfo.Ctx.NewTxn()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error is not checked.
And we can call NewTxn only, it will commit current transaction.

@coocood
Copy link
Member

coocood commented Jan 5, 2017

LGTM

@coocood coocood merged commit 696bea9 into master Jan 5, 2017
@coocood coocood deleted the zimuxia/split-load-data branch January 5, 2017 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants