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

*: add split table syntax to split table region #10553

Merged
merged 16 commits into from
Jun 10, 2019

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented May 21, 2019

What problem does this PR solve?

Please merge :#10409 first.
Add new syntax for split table region.

SPLIT TABLE [table_name] BETWEEN (min_value...) AND (max_value...) REGIONS [region_num]

SPLIT TABLE [table_name] BY [value_lists]

Attention

  • The min and max value count should be 1.
  • The max split index region num currently is 1000 in a single query.

Eg1

split table `t` min (0) max (1000) num 5;

Upper sql will split 5 regions of the table t in range 0~1000. Every region range like below:

Region1: -inf ~ 200

Region2: 200 ~ 400

Region3: 400 ~ 600

Region4: 600 ~ 800

Region5: 800 ~ +inf

Eg2

split table `t` by (100),(200),(300);

Upper sql will split 4 regions of the table t , Every region range like below:

Region1: -inf ~ 100

Region2: 100 ~ 200

Region3: 200 ~ 300

Region4: 300 ~ +inf

What is changed and how it works?

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • Possible performance regression

Related changes

@crazycs520 crazycs520 added type/new-feature require-LGT3 Indicates that the PR requires three LGTM. labels May 21, 2019
executor/builder.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 6, 2019

Codecov Report

Merging #10553 into master will decrease coverage by 0.0235%.
The diff coverage is 63.75%.

@@               Coverage Diff                @@
##             master     #10553        +/-   ##
================================================
- Coverage   79.6029%   79.5794%   -0.0236%     
================================================
  Files           415        415                
  Lines         88101      88254       +153     
================================================
+ Hits          70131      70232       +101     
- Misses        12784      12828        +44     
- Partials       5186       5194         +8

@crazycs520 crazycs520 changed the title *: add split table region syntax *: add split table syntax to split table region Jun 6, 2019
@crazycs520
Copy link
Contributor Author

/run-all-tests

executor/executor_test.go Outdated Show resolved Hide resolved
executor/executor_test.go Outdated Show resolved Hide resolved
executor/split.go Outdated Show resolved Hide resolved
planner/core/planbuilder.go Outdated Show resolved Hide resolved
Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133
Copy link
Member

bb7133 commented Jun 6, 2019

However, we should take care of the English syntax in self-made error messages. Different from code variables/comments, error messages are sent and shown to end-users.

I advise making another PR to fix typos/misspellings in this PR and #10409, for example:

https://github.com/pingcap/tidb/pull/10553/files#diff-ce300a40f7a5a1103912bc56991b9cb8R1794

@crazycs520
Copy link
Contributor Author

/run-all-tests

executor/split.go Outdated Show resolved Hide resolved
executor/split.go Outdated Show resolved Hide resolved
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

rest LGTM

Copy link
Contributor

@winkyao winkyao 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 crazycs520 added the status/LGT2 Indicates that a PR has LGTM 2. label Jun 6, 2019
@crazycs520
Copy link
Contributor Author

/run-all-tests

continue
}
regionIDs = append(regionIDs, regionID)

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 useless line.

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
require-LGT3 Indicates that the PR requires three LGTM. status/LGT2 Indicates that a PR has LGTM 2. type/new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants