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

*: split index region with lower upper syntax #10409

Merged
merged 34 commits into from
Jun 6, 2019
Merged

*: split index region with lower upper syntax #10409

merged 34 commits into from
Jun 6, 2019

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented May 9, 2019 •
edited
Loading

What problem does this PR solve?

Add new syntax for split index region.

SPLIT TABLE [table_name] INDEX [index_name] BETWEEN (lower_value...) AND (upper_value...) REGIONS [region_num]

Attention

  • The lower_value and upper_value value count should less than the index column num.
  • The lower_value and upper_value value count should more than 0.
  • The max split index region num currently is 1000.

Eg1: int type.

split table `t` index `idx1` between (0) and (1000) regions 5;

Upper sql will split 5 regions of the table t index idx1 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: varchar type

split table `t` index `idx1` between ('a') and ('z') regions 26;

Upper sql will split 26 regions.

Region1: -inf ~ b

Region2: b ~ c

...

Region26: y ~ +inf

Eg3: time type.

split table `t` index `idx1` between ('2010-01-01 00:00:00') and ('2020-01-01 00:00:00') regions 10;

Upper sql will split 10 regions.

Region1: -inf ~ 2011-01-01 00:00:00
Region2: 2011-01-01 00:00:00 ~ 2012-01-01 00:00:00

...

Region10: 2011-01-01 00:00:00 ~ +inf

**Eg4: multi-column index. idx1 (col1,col2,col3). **

  • if you only want to split index region by first column col1. You can use SQL like Eg1,Eg2,Eg3. Suppose col1 is int type.

    split table `t` index `idx1` between (-1000) and (1000) regions 5;

  • If you wanto split index regions by first two column. You can use SQL like below.

    split table `t` index `idx1` between (1000, "a") and (1000,"z") regions 26;

What is changed and how it works?

Related Parser PR: pingcap/parser#321

Split num regions between lower and upper value.

Because the key value is byte slice, I cann't do this simple like (max-min) / num .

To solve this problem：

  1. find the longest common prefix of min and max value.
  2. get 8 diff-bytes of min and max from begin, if less than 8, fill with 0xff
  3. convert 8 diff-bytes to uint64. Then use the diff_uint64/num to get the step value.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change

Side effects

Related changes

@crazycs520 crazycs520 added type/new-feature require-LGT3 Indicates that the PR requires three LGTM. labels May 9, 2019
@zhouqiang-cl
Copy link
Contributor

/run-all-tests

1 similar comment
@zhouqiang-cl
Copy link
Contributor

/run-all-tests

@codecov
Copy link

codecov bot commented May 10, 2019 •
edited
Loading

Codecov Report

Merging #10409 into master will decrease coverage by 0.0053%.
The diff coverage is 82.8947%.

@@               Coverage Diff               @@
##             master    #10409        +/-   ##
===============================================
- Coverage   79.5733%   79.568%   -0.0054%     
===============================================
  Files           415       415                
  Lines         87993     88112       +119     
===============================================
+ Hits          70019     70109        +90     
- Misses        12793     12813        +20     
- Partials       5181      5190         +9

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

@winkyao @zimulala @nolouch PTAL

This was referenced May 21, 2019
Merged
Merged
winkyao
winkyao reviewed Jun 4, 2019
View reviewed changes
executor/split.go Outdated Show resolved Hide resolved
winkyao
winkyao reviewed Jun 4, 2019
View reviewed changes
executor/split.go Outdated Show resolved Hide resolved
winkyao
winkyao reviewed Jun 4, 2019
View reviewed changes
executor/split.go Outdated Show resolved Hide resolved
winkyao
winkyao reviewed Jun 4, 2019
View reviewed changes
executor/split.go Outdated Show resolved Hide resolved
winkyao
winkyao reviewed Jun 4, 2019
View reviewed changes
executor/split.go Outdated Show resolved Hide resolved
bb7133
bb7133 reviewed Jun 5, 2019
View reviewed changes
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 bb7133 added the status/LGT2 Indicates that a PR has LGTM 2. label Jun 5, 2019
@crazycs520
Copy link
Contributor Author

/run-all-tests

zimulala
zimulala reviewed Jun 5, 2019
View reviewed changes
planner/core/planbuilder.go Outdated Show resolved Hide resolved
executor/split.go Outdated Show resolved Hide resolved
executor/split.go Outdated Show resolved Hide resolved
zimulala
zimulala reviewed Jun 5, 2019
View reviewed changes
executor/split_test.go Outdated Show resolved Hide resolved
executor/split.go
if err != nil {
return nil, err
}
upperIdxKey, _, err := index.GenIndexKey(e.ctx.GetSessionVars().StmtCtx, e.upper, math.MinInt64, nil)
Copy link
Contributor

@zimulala zimulala Jun 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Why we don't usemath.MaxInt64? Please add comments.

Copy link
Contributor Author

@crazycs520 crazycs520 Jun 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sound reasonable, But I just want to keep consistent with lowerIdxKey here ,I don't like the result of (maxHandleID - minHandleID) != 0 to affect the split point. Use math.MaxInt64 here will get error from TestSplitIndex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already add a comment here.

executor/split_test.go Outdated Show resolved Hide resolved
zimulala
zimulala previously approved these changes Jun 6, 2019
View reviewed changes
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

@zimulala
Copy link
Contributor

zimulala commented Jun 6, 2019

/run-all-tests

@zimulala zimulala added status/LGT3 The PR has already had 3 LGTM. require-LGT3 Indicates that the PR requires three LGTM. and removed require-LGT3 Indicates that the PR requires three LGTM. status/LGT2 Indicates that a PR has LGTM 2. labels Jun 6, 2019
zz-jason
zz-jason reviewed Jun 6, 2019
View reviewed changes
planner/core/planbuilder.go Outdated
return nil, ErrWrongValueCountOnRow.GenWithStackByArgs(i + 1)
}
valueList := make([]types.Datum, 0, len(valuesItem))
convertValue2ColumnType := func(valuesItem []ast.ExprNode) ([]types.Datum, error) {
Copy link
Member

Choose a reason for hiding this comment

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

can you split this function outside the definition of buildSplitRegion()? It's a complicated function with 40 lines, which reduces the readability of buildSplitRegion()

@crazycs520
Copy link
Contributor Author

/run-all-tests

@zz-jason zz-jason merged commit 7bf3d69 into pingcap:master Jun 6, 2019
crazycs520 added a commit to crazycs520/tidb that referenced this pull request Jun 11, 2019
@crazycs520
*: split index region with lower upper syntax (pingcap#10409)
aac260b
@crazycs520 crazycs520 mentioned this pull request Jun 11, 2019
Merged
@jackysp jackysp mentioned this pull request Jun 11, 2019
Closed
jackysp pushed a commit that referenced this pull request Jun 11, 2019
@crazycs520 @jackysp
*: split index region with lower upper syntax (#10409) (#10764)
479c733
crazycs520 added a commit to crazycs520/tidb that referenced this pull request Jun 19, 2019
@crazycs520
*: split index region with lower upper syntax (pingcap#10409)
ddbe86d
@crazycs520 crazycs520 mentioned this pull request Jun 19, 2019
Merged
winkyao pushed a commit that referenced this pull request Jun 20, 2019
@crazycs520 @winkyao
*: split index region with lower upper syntax (#10409) (#10873)
3765b91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winkyao winkyao winkyao left review comments

@bb7133 bb7133 bb7133 left review comments

@zz-jason zz-jason zz-jason approved these changes

@zimulala zimulala zimulala left review comments

Assignees
No one assigned
Labels
require-LGT3 Indicates that the PR requires three LGTM. status/LGT3 The PR has already had 3 LGTM. type/new-feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@crazycs520 @zhouqiang-cl @winkyao @bb7133 @zimulala @zz-jason @zyguan