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

types: restore the field length of a string typed parameter value to the original one #8656

Merged
merged 7 commits into from
Dec 14, 2018
Merged

types: restore the field length of a string typed parameter value to the original one #8656

merged 7 commits into from
Dec 14, 2018

Conversation

dbjoa
Copy link
Contributor

@dbjoa dbjoa commented Dec 11, 2018 •
edited
Loading

What problem does this PR solve?

Fix #8644

What is changed and how it works?

  • Restore the field length of a string typed parameter value to the original one

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • None

Related changes

This change is Reviewable

@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 11, 2018

/run-integration-ddl-test -tidb-test=pr/693

@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 11, 2018

@zhouqiang-cl Would you please check integration-ddl-test error?

ddl_test start

tidb_manager_ttl

10

/home/jenkins/workspace/tidb_ghpr_integration_ddl_test/go

go build github.com/pingcap/tidb/store: no non-test Go files in /home/jenkins/workspace/tidb_ghpr_integration_ddl_test/go/pkg/mod/github.com/pingcap/tidb@v0.0.0-20181207152923-6e2d6c7aa7eb/store

FAIL	github.com/pingcap/tidb-test/ddl_test/ddltest [build failed]

ddl_test end

script returned exit code 1

@dbjoa dbjoa changed the title types: the field length of a string typed parameter value should be restored types: restore the field length of a string typed parameter value to the original one Dec 12, 2018
@zhouqiang-cl
Copy link
Contributor

ok. I will check it

@zhouqiang-cl
Copy link
Contributor

/run-integration-ddl-test -tidb-test=pr/695

@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 12, 2018

@zhouqiang-cl Thx!

@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 12, 2018

@eurekaka @zz-jason PTAL

@jackysp jackysp added type/bugfix This PR fixes a bug. sig/execution SIG execution contribution This PR is from a community contributor. labels Dec 12, 2018
zz-jason
zz-jason reviewed Dec 12, 2018
View reviewed changes
types/field_type.go Outdated
DefaultTypeForValue(value, tp)
if tp.Tp == mysql.TypeVarString {
Copy link
Member

Choose a reason for hiding this comment

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

Should we also check mysql.TypeEnum, mysql.TypeSet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should. The updated PR should use a function to check whether or not the field length of the given value is variable.

eurekaka
eurekaka reviewed Dec 12, 2018
View reviewed changes
types/field_type.go Outdated Show resolved Hide resolved
@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 12, 2018

/run-all-tests

1 similar comment
@zhouqiang-cl
Copy link
Contributor

/run-all-tests

eurekaka
eurekaka reviewed Dec 12, 2018
View reviewed changes
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

LGTM

@eurekaka eurekaka added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 12, 2018
@jackysp
Copy link
Member

jackysp commented Dec 13, 2018

/run-all-tests tidb-test=pr/694

@zhouqiang-cl
Copy link
Contributor

/run-integration-ddl-test

@jackysp
Copy link
Member

jackysp commented Dec 13, 2018

/run-all-tests tidb-test=pr/694

@zhouqiang-cl
Copy link
Contributor

/run-integration-ddl-test tidb-test=pr/694

winoros
winoros reviewed Dec 14, 2018
View reviewed changes
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@jackysp jackysp merged commit 2088caf into pingcap:master Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winoros winoros winoros left review comments

@eurekaka eurekaka eurekaka left review comments

@zz-jason zz-jason zz-jason left review comments

@jackysp jackysp jackysp approved these changes

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. sig/execution SIG execution status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dbjoa @zhouqiang-cl @jackysp @zz-jason @eurekaka @winoros @ngaut