-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
ddl: fix a bug that adding range partition meets error when value is negative #11407
Conversation
PTAL @crazycs520 @zimulala @winkyao |
Codecov Report
@@ Coverage Diff @@
## master #11407 +/- ##
===========================================
Coverage 81.2765% 81.2765%
===========================================
Files 426 426
Lines 92184 92184
===========================================
Hits 74924 74924
Misses 11905 11905
Partials 5355 5355 |
PTAL @crazycs520 @zimulala @winkyao |
for ith, def := range spec.PartDefinitions { | ||
if err := def.Clause.Validate(part.Type, len(part.Columns)); err != nil { | ||
return nil, errors.Trace(err) | ||
} | ||
// For RANGE partition only VALUES LESS THAN should be possible. | ||
clause := def.Clause.(*ast.PartitionDefinitionClauseLessThan) | ||
for _, expr := range clause.Exprs { |
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.
Why just remove it? Could we refine the checking?
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.
As you see, there are no test cases break and I'm adding new test cases, so it's fine to remove 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.
LGTM
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.
LGTM
/run-all-tests |
/run-all-tests |
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.
LGTM
/run-all-tests |
cherry pick to release-3.0 failed |
What problem does this PR solve?
and also this one:
What is changed and how it works?
In this line https://github.com/pingcap/tidb/compare/master...tiancaiamao:fix-add-range-partition?expand=1#diff-edfa54cbe2fdb418f0a77bd0ec4a56f2L3395, we check the expression to be an integer.
However,
expr.GetType()
returns unspecified type and the error is throw out.After removing that, I meet another error, here https://github.com/pingcap/tidb/compare/master...tiancaiamao:fix-add-range-partition?expand=1#diff-edfa54cbe2fdb418f0a77bd0ec4a56f2L2145
checkAddPartitionValue
this function also asserts the partition less than value to be a simple integer,so I replace it with another one.
Check List
Tests
Related changes