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.

Sign up for GitHub

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

Jump to bottom

expression: handle max allowed packet for cast_as_binary #8014

Closed
wants to merge 10 commits into from
Closed

expression: handle max allowed packet for cast_as_binary #8014

wants to merge 10 commits into from

Conversation

zz-jason
Copy link
Member

@zz-jason zz-jason commented Oct 23, 2018 •
edited by c4pt0r
Loading

What problem does this PR solve?

This PR fixes the incorrect result of the sql:

SELECT CAST( 'a' AS BINARY(429496729));

Result of TiDB:

TiDB(localhost:4000) > SELECT CAST( 'a' AS BINARY(429496729));
ERROR 2020 (HY000): Got packet bigger than 'max_allowed_packet' bytes

Result of MySQL:

MySQL(localhost:3306) > SELECT CAST( 'a' AS BINARY(429496729));
+---------------------------------+
| CAST( 'a' AS BINARY(429496729)) |
+---------------------------------+
| NULL                            |
+---------------------------------+
1 row in set, 1 warning (0.01 sec)

MySQL(localhost:3306) > show warnings;
+---------+------+-------------------------------------------------------------------------------------+
| Level   | Code | Message                                                                             |
+---------+------+-------------------------------------------------------------------------------------+
| Warning | 1301 | Result of cast_as_binary() was larger than max_allowed_packet (4194304) - truncated |
+---------+------+-------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

What is changed and how it works?

handle max allowed packet in the cast_as_binary function.

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch

This change is Reviewable

@XuHuaiyu
Copy link
Contributor

conflicts needs be resolved @zz-jason

@zz-jason
Copy link
Member Author

zz-jason commented Nov 5, 2018

/run-all-tests

zimulala
zimulala reviewed Nov 5, 2018
View reviewed changes
ddl/session_pool.go Outdated
@@ -44,6 +45,7 @@ func (sg *sessionPool) get() (sessionctx.Context, error) {
ctx := resource.(sessionctx.Context)
ctx.GetSessionVars().SetStatusFlag(mysql.ServerStatusAutocommit, true)
ctx.GetSessionVars().InRestrictedSQL = true
ctx.GetSessionVars().GetSystemVar(variable.MaxAllowedPacket)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line.

@zz-jason
Copy link
Member Author

zz-jason commented Nov 6, 2018

/run-common-test

XuHuaiyu
XuHuaiyu reviewed Nov 6, 2018
View reviewed changes
expression/integration_test.go
@@ -936,6 +936,9 @@ func (s *testIntegrationSuite) TestStringBuiltin(c *C) {
result = tk.MustQuery("select a,b,concat_ws(',',a,b) from t")
result.Check(testkit.Rows("114.57011441 38.04620115 114.57011441,38.04620115",
"-38.04620119 38.04620115 -38.04620119,38.04620115"))

result = tk.MustQuery("SELECT CAST('a' AS BINARY(67108865));")
result.Check(testkit.Rows("<nil>"))
Copy link
Contributor

Choose a reason for hiding this comment

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

check the result of show warnings here?

@zz-jason
Copy link
Member Author

zz-jason commented Nov 6, 2018

/run-common-test

@zz-jason
Copy link
Member Author

zz-jason commented Nov 9, 2018

/run-common-test

alivxxx
alivxxx reviewed Nov 19, 2018
View reviewed changes
expression/builtin_cast.go
@@ -1021,7 +1034,25 @@ func (b *builtinCastStringAsStringSig) evalString(row chunk.Row) (res string, is
return res, isNull, errors.Trace(err)
}
sc := b.ctx.GetSessionVars().StmtCtx

// isTooLarge := uint64(len(res)) > b.maxAllowedPacket || (b.tp.Flen != types.UnspecifiedLength && uint64(b.tp.Flen) > b.maxAllowedPacket)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove them?

Copy link
Contributor

Choose a reason for hiding this comment

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

PTAL @zz-jason

zimulala
zimulala reviewed Dec 12, 2018
View reviewed changes
expression/builtin_cast.go
@@ -1021,7 +1034,25 @@ func (b *builtinCastStringAsStringSig) evalString(row chunk.Row) (res string, is
return res, isNull, errors.Trace(err)
}
sc := b.ctx.GetSessionVars().StmtCtx

// isTooLarge := uint64(len(res)) > b.maxAllowedPacket || (b.tp.Flen != types.UnspecifiedLength && uint64(b.tp.Flen) > b.maxAllowedPacket)
Copy link
Contributor

Choose a reason for hiding this comment

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

PTAL @zz-jason

expression/builtin_cast.go
if err != nil {
return nil, errors.Trace(err)
}
sig = &builtinCastStringAsStringSig{bf, maxAllowedPacket}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these be extracted into a function?

@zz-jason
Copy link
Member Author

duplicated with #8768, close it now.

@zz-jason zz-jason closed this Dec 29, 2018
@zz-jason zz-jason deleted the bugfix/castAsBinary branch February 17, 2019 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimulala zimulala zimulala left review comments

@XuHuaiyu XuHuaiyu XuHuaiyu left review comments

@alivxxx alivxxx alivxxx left review comments

Assignees
No one assigned
Labels
component/expression type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zz-jason @XuHuaiyu @zimulala @alivxxx