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.

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 create partitioned table with bigint column fail #7520

Merged
merged 4 commits into from
Sep 4, 2018

Conversation

ciscoxll
Copy link
Contributor

@ciscoxll ciscoxll commented Aug 28, 2018

What problem does this PR solve?

Create partitioned table when partitioning key is of type bigint unsigned will fail.

What is changed and how it works?

  • The partitioning key must be an integer, or an expression that returns an integer.
  • For PartitionRangeValue range comparison.
    • Unsigned bigint was converted to uint64 handle.
    • Bigint was converted to int64 handle.

mysql

mysql> create table t1 (a bigint unsigned not null) partition by range(a) (
    ->   partition p0 values less than (10),
    ->   partition p1 values less than (100),
    ->   partition p2 values less than (1000),
    ->   partition p3 values less than (18446744073709551000),
    ->   partition p4 values less than (18446744073709551614)
    -> );
Query OK, 0 rows affected (0.07 sec)

mysql> insert into t1 values (5),(15),(105),(1005);
Query OK, 4 rows affected (0.00 sec)
Records: 4  Duplicates: 0  Warnings: 0

mysql> insert into t1 values (18446744073709551000+1);
Query OK, 1 row affected (0.00 sec)

mysql> insert into t1 values (18446744073709551614-1);
Query OK, 1 row affected (0.00 sec)

master branch

mysql> create table t1 (a bigint unsigned not null) partition by range(a) (
    ->   partition p0 values less than (10),
    ->   partition p1 values less than (100),
    ->   partition p2 values less than (1000),
    ->   partition p3 values less than (18446744073709551000),
    ->   partition p4 values less than (18446744073709551614)
    -> );
ERROR 1493 (HY000): VALUES LESS THAN value must be strictly increasing for each partition

Check List

Tests

  • Unit test

ddl/partition.go Outdated
var offset int
for i, col := range cols {
if col.Name.L == strings.ToLower(pi.Expr) {
offset = i
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need “break” here?

ddl/partition.go Outdated
for i := 0; i < len(defs); i++ {
if strings.EqualFold(defs[i].LessThan[0], partitionMaxValue) {
return errors.Trace(ErrPartitionMaxvalue)
}

currentRangeValue, fromExpr, err := getRangeValue(ctx, tblInfo, defs[i].LessThan[0])
currentRangeValue, fromExpr, err := getRangeValue(ctx, tblInfo, defs[i].LessThan[0], cols, offset)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we replace “cols” and “offset” with “col”?

@ciscoxll
Copy link
Contributor Author

@tiancaiamao @winkyao @zimulala PTAL .

ddl/partition.go Outdated
var prevRangeValue int64

var offset int
for i, col := range cols {
Copy link
Contributor

Choose a reason for hiding this comment

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

Cloud we use "FindCol" directly?

ddl/partition.go Outdated

if currentRangeValue <= prevRangeValue {
return errors.Trace(ErrRangeNotIncreasing)
if col.Tp == mysql.TypeLonglong && mysql.HasUnsignedFlag(col.Flag) {
Copy link
Contributor

Choose a reason for hiding this comment

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

col maybe nil here ?

ddl/partition.go Outdated
@@ -187,13 +187,15 @@ func checkCreatePartitionValue(ctx sessionctx.Context, tblInfo *model.TableInfo,
if strings.EqualFold(defs[len(defs)-1].LessThan[0], partitionMaxValue) {
defs = defs[:len(defs)-1]
}
var prevRangeValue int64

col := table.FindCol(cols, strings.ToUpper(pi.Expr))
Copy link
Contributor

Choose a reason for hiding this comment

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

FindCol use a case-insensitive search, so strings.ToUpper is not necessary

@ciscoxll
Copy link
Contributor Author

@tiancaiamao @winkyao @zimulala PTAL .

@ciscoxll ciscoxll changed the title ddl:create partitioned table with bigint column fail ddl:fix create partitioned table with bigint column fail Aug 30, 2018
ddl/partition.go Outdated
@@ -388,3 +406,15 @@ func checkConstraintIncludePartKey(partkeys []string, constraints map[string]str
}
return true
}

// FindRangePartitionColTp find the type of the partitioning key column type.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/FindRangePartitionColTp find the type of the partitioning key column type./FindRangePartitionColTp finds the type of the partitioning key column type.

@zhexuany
Copy link
Contributor

/run-all-tests

@ciscoxll
Copy link
Contributor Author

/run-all-tests

ddl/partition.go Outdated
if value, err := strconv.ParseInt(str, 10, 64); err == nil {
return value, false, nil
}
func getRangeValue(ctx sessionctx.Context, tblInfo *model.TableInfo, str string, ifUnsignedBigint bool) (interface{}, bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ifUnsignedBigint/unsignedBigint

ddl/partition.go Outdated

// FindRangePartitionColTp finds the type of the partitioning key column type.
// The returned boolean value indicates whether the partitioning key column type is unsigned bigint type.
func FindRangePartitionColTp(cols []*table.Column, pi *model.PartitionInfo) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/FindRangePartitionColTp/isRangePartitionColUnsignedBigint

Copy link
Contributor

Choose a reason for hiding this comment

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

Find column type returns a boolean is weird.

@ciscoxll
Copy link
Contributor Author

@tiancaiamao @winkyao @zimulala PTAL .

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 30, 2018
@ciscoxll
Copy link
Contributor Author

@crazycs520 @winkyao @zimulala PTAL .

@ciscoxll
Copy link
Contributor Author

ciscoxll commented Sep 3, 2018

@crazycs520 @winkyao @zimulala PTAL .

Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@crazycs520
Copy link
Contributor

/run-all-tests

@crazycs520 crazycs520 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 4, 2018
Copy link
Contributor

@zhexuany zhexuany left a comment

Choose a reason for hiding this comment

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

LGTM

@ciscoxll ciscoxll added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Sep 4, 2018
@ciscoxll ciscoxll added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Sep 4, 2018
@ciscoxll ciscoxll merged commit 129f499 into pingcap:master Sep 4, 2018
@ciscoxll ciscoxll deleted the create-partition branch September 4, 2018 08:01
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants