expression: handle max_allowed_packet warnings for repeat function. #7181

hhu-cc · 2018-07-30T05:16:40Z

What have you changed? (mandatory)

Return NULL and a warning when the result of repeat exceeds max_allowed_packetbs.

Before this PR:

tidb> SELECT repeat('h', 18446744073709551617);
+-----------------------------------+
| repeat('h', 18446744073709551617) |
+-----------------------------------+
| NULL                              |
+-----------------------------------+
1 row in set, 1 warning (0.00 sec)

tidb> show warnings;
+---------+------+-----------------------------------------------------------+
| Level   | Code | Message                                                   |
+---------+------+-----------------------------------------------------------+
| Warning | 1292 | Truncated incorrect DECIMAL value: '18446744073709551617' |
+---------+------+-----------------------------------------------------------+
1 row in set (0.00 sec)

After this PR:

tidb> SELECT repeat('h', 18446744073709551617);
+-----------------------------------+
| repeat('h', 18446744073709551617) |
+-----------------------------------+
| NULL                              |
+-----------------------------------+
1 row in set, 2 warnings (2.43 sec)

tidb> show warnings;
+---------+------+------------------------------------------------------------------------------+
| Level   | Code | Message                                                                      |
+---------+------+------------------------------------------------------------------------------+
| Warning | 1292 | Truncated incorrect DECIMAL value: '18446744073709551617'                    |
| Warning | 1301 | Result of repeat() was larger than max_allowed_packet (67108864) - truncated |
+---------+------+------------------------------------------------------------------------------+
2 rows in set (0.00 sec)

What is the type of the changes? (mandatory)

Bug fix (non-breaking change which fixes an issue)

How has this PR been tested? (mandatory)

unit test
explain test

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

Does this PR need to be added to the release notes? (mandatory)

Yes

release note:

Return `NULL` when the result of function `REPEAT` is larger than `max_allowed_packet`

Refer to a related PR or issue link (optional)

to #7153

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

sre-bot · 2018-07-30T05:16:46Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

CLAassistant · 2018-07-30T05:16:48Z

All committers have signed the CLA.

hhu-cc · 2018-07-30T05:19:01Z

/run-all-tests

zz-jason · 2018-07-30T05:26:38Z

@hhxcc Thanks for your contributing! It seems that this PR is blocked by #7171.
@XuHuaiyu PTAL

zz-jason · 2018-07-31T07:13:56Z

@hhxcc, #7171 has been merged into the master branch now, please merge master and resolve conflicts 😄

hhu-cc · 2018-07-31T12:57:46Z

/run-all-tests

hhu-cc · 2018-07-31T12:59:33Z

@zz-jason ok, thanks for notify.

hhu-cc · 2018-07-31T13:20:25Z

/run-all-tests

hhu-cc · 2018-07-31T17:06:22Z

/run-all-tests

zz-jason

LGTM
@XuHuaiyu PTAL

zz-jason · 2018-08-06T03:24:39Z

/run-unit-test

zz-jason · 2018-08-08T12:40:00Z

@hhxcc There are so many irrelevant changes, I think you may need to do some code clean work.

…ssue #7153

hhu-cc · 2018-08-10T01:57:18Z

@zz-jason sorry for that , I have already fixed it.

zz-jason · 2018-08-10T03:08:31Z

LGTM
@XuHuaiyu PTAL

XuHuaiyu · 2018-08-10T03:11:45Z

expression/builtin_string.go

@@ -575,6 +582,7 @@ func (b *builtinRepeatSig) evalString(row chunk.Row) (d string, isNull bool, err
 	if isNull || err != nil {
 		return "", isNull, errors.Trace(err)
 	}
+	byteLength := len(str)


It seems that this is rune length, not byte length?
I think we need to add some test cased for multi-byte characters, like Chinese character.

Len of String will return the byte length, you can check it on https://golang.org/pkg/builtin/#len .
I have added some test of Chinese character.

crazycs520 · 2018-08-14T10:58:13Z

expression/builtin_string_test.go

@@ -398,6 +398,53 @@ func (s *testEvaluatorSuite) TestRepeat(c *C) {
 	c.Assert(v.GetString(), Equals, "")
 }

+func (s *testEvaluatorSuite) TestRepeatSig(c *C) {


Could you refine the test?
Maybe you can keep style with another tests.

crazycs520

LGTM

zz-jason

LGTM

zz-jason · 2018-08-17T11:29:05Z

/run-all-tests

zz-jason · 2018-08-17T15:40:32Z

@hhxcc It would be great if you can cherrypick this PR to the release-2.0 😄

hhu-cc · 2018-08-19T08:51:09Z

@zz-jason ok

…7181) (#7431)

zz-jason mentioned this pull request Jul 30, 2018

need to handle "ER_WARN_ALLOWED_PACKET_OVERFLOWED" for some string functions #7153

Open

13 tasks

shenli added the contribution This PR is from a community contributor. label Jul 30, 2018

zz-jason previously approved these changes Aug 6, 2018

View reviewed changes

hhu-cc dismissed zz-jason’s stale review via d80725c August 8, 2018 08:17

zz-jason added type/bugfix This PR fixes a bug. component/expression labels Aug 8, 2018

hhu-cc and others added 3 commits August 9, 2018 10:01

expression: handle max_allowed_packet warnings for repeat function. i…

ac83164

…ssue #7153

Merge branch 'master' into master

b592e87

Merge branch 'master' into master

2710c48

Merge branch 'master' into master

e16251c

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 10, 2018

XuHuaiyu reviewed Aug 10, 2018

View reviewed changes

hhu-cc and others added 2 commits August 13, 2018 22:25

add more test

315104e

Merge branch 'master' into master

a6eb20b

crazycs520 reviewed Aug 14, 2018

View reviewed changes

refine TestRepeatSig

8ca6783

crazycs520 reviewed Aug 17, 2018

View reviewed changes

zz-jason approved these changes Aug 17, 2018

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 17, 2018

Merge branch 'master' into master

4ba99de

Merge branch 'master' into master

1b78ddf

zz-jason merged commit 2fbd5e7 into pingcap:master Aug 17, 2018

hhu-cc mentioned this pull request Aug 19, 2018

expression: handle max_allowed_packet warnings for repeat function. c… #7431

Merged

ngaut pushed a commit that referenced this pull request Aug 22, 2018

expression: handle max_allowed_packet warnings for repeat function. (#…

fc14119

…7181) (#7431)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: handle max_allowed_packet warnings for repeat function. #7181

expression: handle max_allowed_packet warnings for repeat function. #7181

hhu-cc commented Jul 30, 2018

sre-bot commented Jul 30, 2018

CLAassistant commented Jul 30, 2018 •

edited

Loading

hhu-cc commented Jul 30, 2018

zz-jason commented Jul 30, 2018

zz-jason commented Jul 31, 2018

hhu-cc commented Jul 31, 2018

hhu-cc commented Jul 31, 2018

hhu-cc commented Jul 31, 2018

hhu-cc commented Jul 31, 2018

zz-jason left a comment

zz-jason commented Aug 6, 2018

zz-jason commented Aug 8, 2018

hhu-cc commented Aug 10, 2018

zz-jason commented Aug 10, 2018

XuHuaiyu Aug 10, 2018

hhu-cc Aug 13, 2018 •

edited

Loading

crazycs520 Aug 14, 2018

hhu-cc Aug 14, 2018

crazycs520 left a comment

zz-jason left a comment

zz-jason commented Aug 17, 2018

zz-jason commented Aug 17, 2018

hhu-cc commented Aug 19, 2018

expression: handle max_allowed_packet warnings for repeat function. #7181

expression: handle max_allowed_packet warnings for repeat function. #7181

Conversation

hhu-cc commented Jul 30, 2018

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

sre-bot commented Jul 30, 2018

CLAassistant commented Jul 30, 2018 • edited Loading

hhu-cc commented Jul 30, 2018

zz-jason commented Jul 30, 2018

zz-jason commented Jul 31, 2018

hhu-cc commented Jul 31, 2018

hhu-cc commented Jul 31, 2018

hhu-cc commented Jul 31, 2018

hhu-cc commented Jul 31, 2018

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Aug 6, 2018

zz-jason commented Aug 8, 2018

hhu-cc commented Aug 10, 2018

zz-jason commented Aug 10, 2018

XuHuaiyu Aug 10, 2018

Choose a reason for hiding this comment

hhu-cc Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

crazycs520 Aug 14, 2018

Choose a reason for hiding this comment

hhu-cc Aug 14, 2018

Choose a reason for hiding this comment

crazycs520 left a comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Aug 17, 2018

zz-jason commented Aug 17, 2018

hhu-cc commented Aug 19, 2018

CLAassistant commented Jul 30, 2018 •

edited

Loading

hhu-cc Aug 13, 2018 •

edited

Loading