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

executor: support load data with ignore lines #7576

Merged
merged 7 commits into from
Sep 6, 2018

Conversation

mccxj
Copy link
Contributor

@mccxj mccxj commented Sep 1, 2018

What problem does this PR solve?

For #7476

What is changed and how it works?

  • support ‘IGNORE NUM LINES’ in parser.y
  • check IgnoreLines in LoadDataInfo#InsertData

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported variable/fields change

@shenli
Copy link
Member

shenli commented Sep 2, 2018

@mccxj Thanks!

@shenli shenli added the contribution This PR is from a community contributor. label Sep 2, 2018
parser/parser.y Outdated
@@ -809,6 +809,7 @@ import (
TableOptimizerHintOpt "Table level optimizer hint"
TableOptimizerHints "Table level optimizer hints"
TableOptimizerHintList "Table level optimizer hint list"
IgnoreLines "Ignore num(int) lines"
Copy link
Member

Choose a reason for hiding this comment

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

Keep alignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

c.Assert(ok, IsTrue)
defer ctx.SetValue(executor.LoadDataVarKey, nil)
c.Assert(ld, NotNil)
// test escape
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by "test escape"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy comment by mistake.

parser/parser.y Outdated
@@ -802,6 +802,7 @@ import (
OptBinMod "Optional BINARY mode"
OptCharset "Optional Character setting"
OptCollate "Optional Collate setting"
IgnoreLines "Ignore num(int) lines"
Copy link
Member

Choose a reason for hiding this comment

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

Please keep alignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

{
$$ = uint64(0)
}
| "IGNORE" NUM "LINES"
Copy link
Member

Choose a reason for hiding this comment

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

In MySQL, a negative number will casue syntax error. Could you add a test case about this?

mysql> load data local infile '/tmp/t.csv' into table t ignore -1 lines;
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '-1 lines' at line 1
mysql> load data local infile '/tmp/t.csv' into table t ignore 1 lines;
ERROR 1148 (42000): The used command is not allowed with this MySQL version
mysql> quit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

testcase added.

Copy link
Member

Choose a reason for hiding this comment

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

Got it.

Copy link
Member

@shenli shenli left a comment

Choose a reason for hiding this comment

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

LGTM

@shenli
Copy link
Member

shenli commented Sep 6, 2018

/run-all-tests

@shenli shenli added status/all tests passed status/LGT2 Indicates that a PR has LGTM 2. labels Sep 6, 2018
@shenli shenli merged commit b2bfd8f into pingcap:master Sep 6, 2018
@shenli
Copy link
Member

shenli commented Sep 6, 2018

@mccxj Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2. type/compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants