executor: fix csv parser #9005

imtbkcat · 2019-01-10T08:21:32Z

What problem does this PR solve?

TiDB will panic when encounter irregular csv format, like this.

\N
"\N",
"NULL",
NULL

Test SQL:

create table test (i int default null);
load data local infile 'abc.csv' into table test FIELDS TERMINATED BY ',' enclosed by '"';

This is because csv file above mixed enclosed words and unenclosed words.

another case will cause panic is shown below.

abc,123,
"def",456
hij,"789",

TiDB:

mysql> create table test (str varchar(10) default null, i int default null);
Query OK, 0 rows affected (0.01 sec)

mysql> load data local infile '~/test.csv' into table test FIELDS TERMINATED BY ',' enclosed by '"';
ERROR 2013 (HY000): Lost connection to MySQL server during query

MySQL:

mysql> create table test (str varchar(10) default null, i int default null);
Query OK, 0 rows affected (0.02 sec)

mysql> load data local infile '~/test.csv' into table test FIELDS TERMINATED BY ',' enclosed by '"';
Query OK, 3 rows affected (0.00 sec)
Records: 3  Deleted: 0  Skipped: 0  Warnings: 0

mysql> select * from test;
+------+------+
| str  | i    |
+------+------+
| abc  |  123 |
| def  |  456 |
| hij  |  789 |
+------+------+
3 rows in set (0.00 sec)

What is changed and how it works?

Restructuring getFieldsFromLine function.

Check List

Tests

Unit test
Manual test

Code changes

Has exported function/method change

Side effects

Increased code complexity

Related changes

imtbkcat · 2019-01-10T09:21:38Z

/run-all-tests

codecov-io · 2019-01-10T09:26:00Z

Codecov Report

Merging #9005 into master will increase coverage by 0.01%.
The diff coverage is 82.72%.

@@            Coverage Diff             @@
##           master    #9005      +/-   ##
==========================================
+ Coverage   67.16%   67.17%   +0.01%     
==========================================
  Files         371      371              
  Lines       76393    76486      +93     
==========================================
+ Hits        51311    51383      +72     
- Misses      20494    20511      +17     
- Partials     4588     4592       +4

Impacted Files	Coverage Δ
executor/load_data.go	`81.48% <82.72%> (-0.88%)`	⬇️
executor/join.go	`77.92% <0%> (-0.52%)`	⬇️
executor/distsql.go	`72.53% <0%> (-0.47%)`	⬇️
executor/executor.go	`67.08% <0%> (+0.14%)`	⬆️
expression/schema.go	`94.95% <0%> (+0.84%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dca815c...0f0b27b. Read the comment docs.

jackysp

It is better to add the test cases which this PR could fix.

jackysp · 2019-01-11T02:02:50Z

executor/load_data.go

-		sep = append(sep, e.FieldsInfo.Enclosed)
-		sep = append(sep, e.FieldsInfo.Terminated...)
-		sep = append(sep, e.FieldsInfo.Enclosed)
+	var (


There are so many lines in this function. Please split it into pieces.

imtbkcat · 2019-01-11T06:00:20Z

/run-all-tests

imtbkcat · 2019-01-11T07:12:19Z

/run-all-tests

imtbkcat · 2019-01-11T07:26:12Z

/run-unit-test

tiancaiamao

LGTM

tiancaiamao · 2019-01-14T09:12:52Z

server/server_test.go

+	fp, err = os.Create(path)
+	c.Assert(err, IsNil)
+	c.Assert(fp, NotNil)
+


You can os.Remove(path) here immediately.

The next test case still use this file.

tiancaiamao · 2019-01-14T09:32:52Z

executor/load_data.go

+}
+
+func (w *fieldWriter) GetField() (bool, field) {
+	// the bool return value implies whether is at the end of line.


s/the/The

whether ?? is at the end of line

tiancaiamao · 2019-01-14T11:06:45Z

executor/load_data.go

+			w.rollback()
+		}
+	}
+	for {


Now it's complex enough and more difficult to maintain.
If we meet some error next time, I'll consider use some more general method instead of hard written those things.

jackysp · 2019-01-14T11:38:17Z

executor/load_data.go

+	w.term = term
+}
+
+func (w *fieldWriter) rollback() {


It is better to change the function name.

jackysp · 2019-01-14T11:41:34Z

executor/executor_pkg_test.go

@@ -198,6 +198,11 @@ func (s *testExecSuite) TestGetFieldsFromLine(c *C) {
 			`"\0\b\n\r\t\Z\\\  \c\'\""`,
 			[]string{string([]byte{0, '\b', '\n', '\r', '\t', 26, '\\', ' ', ' ', 'c', '\'', '"'})},
 		},
+		// Test mixed.
+		{
+			`"123",456,"\t7890",abcd`,


It is better to add more test cases to guarantee the behavior we support.

jackysp

rest LGTM

imtbkcat · 2019-01-15T06:53:56Z

/run-all-tests

jackysp

LGTM

imtbkcat added type/bugfix This PR fixes a bug. sig/execution SIG execution labels Jan 10, 2019

imtbkcat requested review from tiancaiamao and jackysp January 10, 2019 08:21

imtbkcat added the status/all tests passed label Jan 10, 2019

jackysp reviewed Jan 11, 2019

View reviewed changes

zz-jason requested review from XuHuaiyu and qw4990 January 14, 2019 02:58

tiancaiamao reviewed Jan 14, 2019

View reviewed changes

jackysp reviewed Jan 14, 2019

View reviewed changes

executor/load_data.go Outdated

w.term = term

}

func (w *fieldWriter) rollback() {

Copy link

Member

jackysp Jan 14, 2019

Choose a reason for hiding this comment

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

It is better to change the function name.

jackysp reviewed Jan 14, 2019

View reviewed changes

imtbkcat added 10 commits January 15, 2019 14:49

fix load data

41724fc

add comment for review

a689a76

fix go lint

f074157

fix ci

f55713f

fix go lont

cbab6b7

made code more readable

043eb85

fix golint

9a4fe9c

add more test case

2b0e992

add comment

48356f5

add unit test

0f0b27b

imtbkcat force-pushed the fixloaddata branch from fa7320d to 0f0b27b Compare January 15, 2019 06:49

jackysp approved these changes Jan 15, 2019

View reviewed changes

Merge branch 'master' into fixloaddata

d5eb2f7

jackysp merged commit 33b4c3e into pingcap:master Jan 15, 2019

imtbkcat pushed a commit to imtbkcat/tidb that referenced this pull request Apr 25, 2019

executor: fix csv parser (pingcap#9005)

6d73c90

imtbkcat mentioned this pull request Apr 25, 2019

executor: fix csv parser (#9005) #10269

Merged

jackysp pushed a commit that referenced this pull request Apr 26, 2019

executor: fix csv parser (#9005) (#10269)

b51f531

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: fix csv parser #9005

executor: fix csv parser #9005

imtbkcat commented Jan 10, 2019 •

edited

Loading

imtbkcat commented Jan 10, 2019

codecov-io commented Jan 10, 2019 •

edited

Loading

jackysp left a comment

jackysp Jan 11, 2019

imtbkcat commented Jan 11, 2019

imtbkcat commented Jan 11, 2019

imtbkcat commented Jan 11, 2019

tiancaiamao left a comment

tiancaiamao Jan 14, 2019

imtbkcat Jan 14, 2019

tiancaiamao Jan 14, 2019

tiancaiamao Jan 14, 2019

jackysp Jan 14, 2019

jackysp Jan 14, 2019

jackysp left a comment

imtbkcat commented Jan 15, 2019

jackysp left a comment

executor: fix csv parser #9005

executor: fix csv parser #9005

Conversation

imtbkcat commented Jan 10, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

imtbkcat commented Jan 10, 2019

codecov-io commented Jan 10, 2019 • edited Loading

Codecov Report

jackysp left a comment

Choose a reason for hiding this comment

jackysp Jan 11, 2019

Choose a reason for hiding this comment

imtbkcat commented Jan 11, 2019

imtbkcat commented Jan 11, 2019

imtbkcat commented Jan 11, 2019

tiancaiamao left a comment

Choose a reason for hiding this comment

tiancaiamao Jan 14, 2019

Choose a reason for hiding this comment

imtbkcat Jan 14, 2019

Choose a reason for hiding this comment

tiancaiamao Jan 14, 2019

Choose a reason for hiding this comment

tiancaiamao Jan 14, 2019

Choose a reason for hiding this comment

jackysp Jan 14, 2019

Choose a reason for hiding this comment

jackysp Jan 14, 2019

Choose a reason for hiding this comment

jackysp left a comment

Choose a reason for hiding this comment

imtbkcat commented Jan 15, 2019

jackysp left a comment

Choose a reason for hiding this comment

imtbkcat commented Jan 10, 2019 •

edited

Loading

codecov-io commented Jan 10, 2019 •

edited

Loading