-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
expression: handle max_allowed_packet warnings for to_base64 function. #7266
Changes from 4 commits
5bb5cf7
33a10e3
ea06d28
9e310ec
8c54930
abc9329
9700a53
1bbf7f4
0467a1f
7f158ac
67ea3e2
78c3716
2c6dd39
67984ea
60f73b5
055c2af
857951c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1862,6 +1862,84 @@ func (s *testEvaluatorSuite) TestToBase64(c *C) { | |
c.Assert(err, IsNil) | ||
} | ||
|
||
func (s *testEvaluatorSuite) TestToBase64Sig(c *C) { | ||
colTypes := []*types.FieldType{ | ||
{Tp: mysql.TypeVarchar}, | ||
} | ||
|
||
tests := []struct { | ||
args string | ||
expect string | ||
isNil bool | ||
getErr bool | ||
maxAllowPacket uint64 | ||
}{ | ||
{"abc", "YWJj", false, false, 16}, | ||
{"abc", "", true, false, 15}, | ||
{ | ||
"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/", | ||
"QUJDREVGR0hJSktMTU5PUFFSU1RVVldYWVphYmNkZWZnaGlqa2xtbm9wcXJzdHV2d3h5ejAxMjM0\nNTY3ODkrLw==", | ||
false, | ||
false, | ||
356, | ||
}, | ||
{ | ||
"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/", | ||
"", | ||
true, | ||
false, | ||
355, | ||
}, | ||
{ | ||
"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/", | ||
"QUJDREVGR0hJSktMTU5PUFFSU1RVVldYWVphYmNkZWZnaGlqa2xtbm9wcXJzdHV2d3h5ejAxMjM0\nNTY3ODkrL0FCQ0RFRkdISUpLTE1OT1BRUlNUVVZXWFlaYWJjZGVmZ2hpamtsbW5vcHFyc3R1dnd4\neXowMTIzNDU2Nzg5Ky9BQkNERUZHSElKS0xNTk9QUVJTVFVWV1hZWmFiY2RlZmdoaWprbG1ub3Bx\ncnN0dXZ3eHl6MDEyMzQ1Njc4OSsv", | ||
false, | ||
false, | ||
1036, | ||
}, | ||
{ | ||
"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/", | ||
"", | ||
true, | ||
false, | ||
1035, | ||
}, | ||
} | ||
|
||
args := []Expression{ | ||
&Column{Index: 0, RetType: colTypes[0]}, | ||
} | ||
|
||
warningCount := 0 | ||
|
||
for _, test := range tests { | ||
resultType := &types.FieldType{Tp: mysql.TypeVarchar, Flen: base64NeededEncodedLength(len(test.args))} | ||
base := baseBuiltinFunc{args: args, ctx: s.ctx, tp: resultType} | ||
toBase64 := &builtinToBase64Sig{base, test.maxAllowPacket} | ||
|
||
input := chunk.NewChunkWithCapacity(colTypes, 1) | ||
input.AppendString(0, test.args) | ||
res, isNull, err := toBase64.evalString(input.GetRow(0)) | ||
if test.getErr { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all the added |
||
c.Assert(err, NotNil) | ||
} else { | ||
c.Assert(err, IsNil) | ||
} | ||
if test.isNil { | ||
c.Assert(isNull, IsTrue) | ||
warningCount += 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we reset the warning appended to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
} else { | ||
c.Assert(isNull, IsFalse) | ||
} | ||
c.Assert(res, Equals, test.expect) | ||
} | ||
warnings := s.ctx.GetSessionVars().StmtCtx.GetWarnings() | ||
c.Assert(len(warnings), Equals, warningCount) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
for _, warn := range warnings { | ||
c.Assert(terror.ErrorEqual(errWarnAllowedPacketOverflowed, warn.Err), IsTrue) | ||
} | ||
} | ||
|
||
func (s *testEvaluatorSuite) TestStringRight(c *C) { | ||
defer testleak.AfterTest(c)() | ||
fc := funcs[ast.Right] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not correct to use
Flen
to decide whether the result ofToBase64
exceedsmax_allowed_packet
, I think we can learn from MySQL:The above code is taken from: https://github.com/mysql/mysql-server/blob/5.7/sql/item_strfunc.cc#L690
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, thanks, I will correct it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @supernan1994 any update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry!!! I had a bussiness trip and was back yesterday, I will take a look at it tonight and tomorrow night @zz-jason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄 take your time, it's just a just a friendly Ping