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

types: fix delete error when convert string to float or int #10861

Merged
merged 5 commits into from
Jul 16, 2019

Conversation

DQinYuan
Copy link
Contributor

@DQinYuan DQinYuan commented Jun 19, 2019

What problem does this PR solve?

To fix the issue #10806

When delete from a varchar column only with empty string and a condition compare with a integer or float, tidb will return a "Data Truncated Error" and mysql(both 5.x and 8.x) will only return "Query OK, 0 rows affected (0.01 sec) "

What is changed and how it works?

Before:

tidb>create table t (a varchar(30));
tidb>insert into t values ('');
tidb>delete from t where a=10;
ERROR 1265 (01000): Data Truncated

After:

tidb>create table t (a varchar(30));
tidb>insert into t values ('');
tidb>delete from t where a=10;
Query OK, 0 rows affected (0.01 sec) 

modify method getValidFloatPrefix in types/convert.go

When stmt context is "delete"(i.e. sc.InDeleteStmt is true) and string is empty, getValidFloatPrefix directly return "0" and nil error.

func getValidFloatPrefix(sc *stmtctx.StatementContext, s string) (valid string, err error) {
	if sc.InDeleteStmt && s == "" {
		return "0", nil
	}
......

Check List

Tests

  • Unit test

@CLAassistant
Copy link

CLAassistant commented Jun 19, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jun 19, 2019

Codecov Report

Merging #10861 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #10861   +/-   ##
===========================================
  Coverage   81.2876%   81.2876%           
===========================================
  Files           423        423           
  Lines         90133      90133           
===========================================
  Hits          73267      73267           
  Misses        11561      11561           
  Partials       5305       5305

@DQinYuan DQinYuan force-pushed the fix-delete-bug branch 2 times, most recently from 47619a2 to 6e87e72 Compare June 22, 2019 09:08
@DQinYuan DQinYuan requested a review from alivxxx June 22, 2019 09:09
Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@alivxxx alivxxx added type/bugfix This PR fixes a bug. sig/execution SIG execution status/LGT1 Indicates that a PR has LGTM 1. labels Jun 24, 2019
@alivxxx alivxxx requested review from qw4990 and XuHuaiyu June 24, 2019 07:19
@@ -580,6 +580,10 @@ func ConvertJSONToDecimal(sc *stmtctx.StatementContext, j json.BinaryJSON) (*MyD

// getValidFloatPrefix gets prefix of string which can be successfully parsed as float.
func getValidFloatPrefix(sc *stmtctx.StatementContext, s string) (valid string, err error) {
if sc.InDeleteStmt && s == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is used in StrToUint, maybe not only delete statement?

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 condition statementContext.InDeleteStmt == true will keep it only work in delete statement.

And in delete statement, empty string can always convert to 0 without error and warning.(according to behaviour of mysql 5.x and 8.x ).

But in other statement like select, tidb is consistent with mysql(all with warning), so I restrict it in delete statement context.

@DQinYuan DQinYuan requested a review from winkyao June 25, 2019 05:04
Copy link
Contributor

@Deardrops Deardrops left a comment

Choose a reason for hiding this comment

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

I suggest do not add more parameter for func testStrToFloat, considering 2 points:

  1. This new parameter inDeleteStmt only use in one test case - String is empty. We don't need to bring this parameter to other test case, which is this PR has done.
  2. The test cases of TestStrToInt and TestStrToUInt are missing. According to the Issue's description Inconsisitent with Mysql When a varchar column only have empty string #10806, we actually need these test case.

In my view, we could add a new test function for the case of this issue

func testDeleteEmptyStringError(c *C) {
	sc := new(stmtctx.StatementContext)
	sc.InDeleteStmt = true

	str := ""
	expect := 0

	val, err := StrToInt(sc, str)
	c.Assert(err, IsNil)
	c.Assert(val, Equals, int64(expect))

	val1, err := StrToUint(sc, str)
	c.Assert(err, IsNil)
	c.Assert(val1, Equals, uint64(expect))

	val2, err := StrToFloat(sc, str)
	c.Assert(err, IsNil)
	c.Assert(val2, Equals, float64(expect))
}

and call the new test function in the end of func TestStrToNum

	testStrToFloat(c, "-1e649", -math.MaxFloat64, true, ErrTruncatedWrongVal)
	testStrToFloat(c, "-1e649", -math.MaxFloat64, false, nil)

+	// for issue #10806
+	testDeleteEmptyStringError(c)
}

@DQinYuan
Copy link
Contributor Author

@Deardrops You are right, I have corrected unit test in this pr.

@DQinYuan DQinYuan requested a review from Deardrops June 28, 2019 03:05
@Deardrops
Copy link
Contributor

LGTM

zz-jason
zz-jason previously approved these changes Jul 15, 2019
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason
Copy link
Member

@DQinYuan Sorry for the late review. Could you update the PR with the latest master branch and resolve the conflicts?

@zz-jason zz-jason added needs-cherry-pick-2.1 and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 15, 2019
@zz-jason zz-jason added needs-cherry-pick-3.0 status/LGT2 Indicates that a PR has LGTM 2. labels Jul 15, 2019
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants