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

util/types:Handle out of range value when load data #3508

Merged
merged 6 commits into from
Jun 20, 2017

Conversation

louishust
Copy link
Contributor

When insert an overflow string like "1343545435346432587475"
to MySQL int type column, tidb will convert to max int64
instead of max int.

When insert an overflow string like "1343545435346432587475"
to MySQL int type column, tidb will convert to max int64
instead of max int.
@louishust
Copy link
Contributor Author

solve bug #3024

@@ -131,6 +133,15 @@ func testDatumToInt64(c *C, val interface{}, expect int64) {
c.Assert(b, Equals, expect)
}

func testDatumToInt64WithTargetType(c *C, val interface{}, tp byte, expectErr Checker, expectVal int64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can add this test it in util/types/convert_test.go,
thus ToInt64WithTp , testDatumToInt64WithTargetType and testDatumToInt64WithTargetType can be removed.

@XuHuaiyu
Copy link
Contributor

PTAL @zimulala

@shenli
Copy link
Member

shenli commented Jun 19, 2017

@louishust Should we distinguish between loading data and insert statement?

mysql> create table t (a int, b int not null);
Query OK, 0 rows affected (0.01 sec)

mysql> insert into t values (143, 1343545435346432587475);
ERROR 1264 (22003): Out of range value for column 'b' at row 1
mysql>

@louishust
Copy link
Contributor Author

@shenli

mysql> set sql_mode="";
Query OK, 0 rows affected (0.01 sec)

mysql> insert into t1 values(1, "1343545435346432587475");
Query OK, 1 row affected, 1 warning (0.00 sec)

mysql> select * from t1;
+------+------------+
| a    | b          |
+------+------------+
|  143 | 2147483647 |
|    1 | 2147483647 |
+------+------------+
2 rows in set (0.00 sec)

I think the sql_mode is modified at load data command internal in MySQL.
I will check that in MySQL later.

@louishust
Copy link
Contributor Author

@shenli

I checked MySQL code and find the following code in sql/sql_yacc.yy:

load:
.............
        if (lex->local_file && lex->duplicates == DUP_ERROR)
              lex->set_ignore(true);

So if load data with local keyword, the load will treat truncate error as warnings.

And tidb only support load data local infile, so should always ignore the error,
and tidb already handled this situation in executor/write.go:

func (e *LoadDataInfo) insertData(cols []string) {
	for i := 0; i < len(e.row); i++ {
		if i >= len(cols) {
			e.row[i].SetString("")
			continue
		}
		e.row[i].SetString(cols[i])
	}
	row, err := e.insertVal.fillRowData(e.columns, e.row, true)

The fillRowData's third parameter is true.

@shenli
Copy link
Member

shenli commented Jun 19, 2017

@zimulala PTAL

@hanfei1991
Copy link
Member

LGTM

@hanfei1991 hanfei1991 added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 20, 2017
@zimulala zimulala added contribution This PR is from a community contributor. and removed contribution This PR is from a community contributor. labels Jun 20, 2017
@zimulala
Copy link
Contributor

zimulala commented Jun 20, 2017

If using the statement of load data infile behavior is like sql_mode is empty behavior, then we can directly set sql_mode value to empty when handling the statement of load data infile. Because the function considers the value of sql_mode.
And please add more tests to verify your claim.

@zimulala
Copy link
Contributor

@louishust

@louishust
Copy link
Contributor Author

@zimulala Ignore the sql_mode, only LOCAL keyword matters.

@@ -1327,6 +1327,8 @@ func (d *Datum) toSignedInteger(sc *variable.StatementContext, tp byte) (int64,
case KindString, KindBytes:
iVal, err := StrToInt(sc, d.GetString())
if err != nil {
// adjust the overflow value
iVal, _ = ConvertIntToInt(iVal, lowerBound, upperBound, tp)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, I forgot StrToInt implementation details.
You can consider it like the logical.@louishust

@louishust
Copy link
Contributor Author

@zimulala modified the logic

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 20, 2017
@hanfei1991 hanfei1991 merged commit 9233ccf into pingcap:master Jun 20, 2017
@louishust louishust deleted the load branch June 20, 2017 11:54
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants