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

table partition: fix CREATE partitioned table fails check for unsigned int column definition #35876

Closed
wants to merge 6 commits into from
Closed

table partition: fix CREATE partitioned table fails check for unsigned int column definition #35876

wants to merge 6 commits into from

Conversation

ymkzpx
Copy link
Contributor

@ymkzpx ymkzpx commented Jul 1, 2022

What problem does this PR solve?

Issue Number: close #35827

Problem Summary:

What is changed and how it works?

Check List

Tests

  • Unit test

Release note

None.

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 needs-cherry-pick-release-5.2 needs-cherry-pick-release-5.3 Type: Need cherry pick to release-5.3 needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. needs-cherry-pick-6.0 needs-cherry-pick-release-6.1 Should cherry pick this PR to release-6.1 branch. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 1, 2022
@ymkzpx
Copy link
Contributor Author

ymkzpx commented Jul 1, 2022

/run-unit-test

ymkzpx
ymkzpx commented Jul 1, 2022
View reviewed changes
ddl/partition.go Outdated Show resolved Hide resolved
@ymkzpx
Copy link
Contributor Author

ymkzpx commented Jul 1, 2022

/run-unit-test

1 similar comment
@ymkzpx
Copy link
Contributor Author

ymkzpx commented Jul 1, 2022

/run-unit-test

mjonss
mjonss reviewed Jul 1, 2022
View reviewed changes
ddl/partition.go
@@ -660,9 +660,11 @@ func buildRangePartitionDefinitions(ctx sessionctx.Context, defs []*ast.Partitio

func checkPartitionValuesIsInt(ctx sessionctx.Context, def *ast.PartitionDefinition, exprs []ast.ExprNode, tbInfo *model.TableInfo) error {
tp := types.NewFieldType(mysql.TypeLonglong)
if isColUnsigned(tbInfo.Columns, tbInfo.Partition) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the function isColUnsigned is not working as expected, please remove it (or fix it).

Copy link
Contributor

Choose a reason for hiding this comment

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

Please address this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, TiDB has a problem recognizing unsigned data type. If we change the condition substring to equal, for example, an expression like id-10, id is unsigned data type, TiDB can't handle.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any suggestions on how to solve this? I still think we should not keep the broken isColUnsigned, we should either fix it or remove it ;)
In some edge cases it is OK to not be 100% compatible with MySQL if we have a good reason not to.

@ymkzpx
Copy link
Contributor Author

ymkzpx commented Jul 2, 2022 •
edited
Loading

@mjonss If the contain is changed to equal, the case can't be handled, unit-test will fail, as shown below.
image

@mjonss
Copy link
Contributor

mjonss commented Jul 4, 2022

@mjonss If the contain is changed to equal, the case can't be handled, unit-test will fail, as shown below. image

So it will get another error? What would you suggest to handle this, accept the new error or fix the code to retain the old error?

@ti-chi-bot ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 5, 2022
@ymkzpx
Copy link
Contributor Author

ymkzpx commented Jul 5, 2022 •
edited
Loading

So it will get another error? What would you suggest to handle this, accept the new error or fix the code to retain the old error?

@mjonss
I checked the code and found that if expression is less than 0, TiDB currently does not check for unsigned data type, which may cause bug, so i updated the code to fix this, PTAL.

mjonss
mjonss reviewed Jul 5, 2022
View reviewed changes
Copy link
Contributor

@mjonss mjonss left a comment

Choose a reason for hiding this comment

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

LGTM, just two minor things to address.

ddl/partition.go Outdated Show resolved Hide resolved
ddl/partition.go
@@ -660,9 +660,11 @@ func buildRangePartitionDefinitions(ctx sessionctx.Context, defs []*ast.Partitio

func checkPartitionValuesIsInt(ctx sessionctx.Context, def *ast.PartitionDefinition, exprs []ast.ExprNode, tbInfo *model.TableInfo) error {
tp := types.NewFieldType(mysql.TypeLonglong)
if isColUnsigned(tbInfo.Columns, tbInfo.Partition) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please address this.

mjonss
mjonss reviewed Jul 6, 2022
View reviewed changes
errno/errcode.go
@@ -822,6 +822,7 @@ const (
ErrMustChangePasswordLogin = 1862
ErrRowInWrongPartition = 1863
ErrErrorLast = 1863
ErrUnsignedNotLessThanZero = 1865
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ErrUnsignedNotLessThanZero = 1865
ErrUnsignedLessThanZero = 1865

ddl/partition.go
@@ -660,9 +660,11 @@ func buildRangePartitionDefinitions(ctx sessionctx.Context, defs []*ast.Partitio

func checkPartitionValuesIsInt(ctx sessionctx.Context, def *ast.PartitionDefinition, exprs []ast.ExprNode, tbInfo *model.TableInfo) error {
tp := types.NewFieldType(mysql.TypeLonglong)
if isColUnsigned(tbInfo.Columns, tbInfo.Partition) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any suggestions on how to solve this? I still think we should not keep the broken isColUnsigned, we should either fix it or remove it ;)
In some edge cases it is OK to not be 100% compatible with MySQL if we have a good reason not to.

@mjonss
Copy link
Contributor

mjonss commented Aug 2, 2022

Maybe something like this?

@ymkzpx ymkzpx closed this Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjonss mjonss mjonss left review comments

Assignees
No one assigned
Labels
needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 needs-cherry-pick-release-5.2 needs-cherry-pick-release-5.3 Type: Need cherry pick to release-5.3 needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. needs-cherry-pick-release-6.1 Should cherry pick this PR to release-6.1 branch. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CREATE partitioned table fails check for unsigned int column definition
3 participants
@ymkzpx @ti-chi-bot @mjonss