-
Notifications
You must be signed in to change notification settings - Fork 724
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
region: check if the query startkey and endkey specify an uncovered region #8790
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Boyang Lyu <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @JackL9u. Thanks for your PR. I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8790 +/- ##
==========================================
- Coverage 75.71% 75.64% -0.08%
==========================================
Files 461 461
Lines 72285 72329 +44
==========================================
- Hits 54731 54712 -19
- Misses 14045 14116 +71
+ Partials 3509 3501 -8
Flags with carried forward coverage won't be shown. Click here to find out more. |
/ok-to-test |
Can you add some description to explain your work? For example:
This is very helpful to review your code |
comments updated with explanations. |
startInAnInterval := false | ||
if startItem != nil { | ||
startInAnInterval = (bytes.Compare(startItem.GetStartKey(), startKey) <= 0) && (bytes.Compare(startKey, startItem.GetEndKey()) <= 0) |
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.
If the startItem != nil
(L1799), startInAnInterval is true?
} | ||
endInAnInterval := false | ||
if endItem != nil { | ||
endInAnInterval = (bytes.Compare(endItem.GetStartKey(), endKey) <= 0) && (bytes.Compare(endKey, endItem.GetEndKey()) <= 0) |
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.
ditto
eit = r.tree.tree.Len() | ||
endInAnInterval = false | ||
if endItem != nil { | ||
endInAnInterval = (bytes.Compare(endItem.GetEndKey(), endKey) <= 0) && (bytes.Compare(endKey, endItem.GetEndKey()) <= 0) |
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 do you check endkey when it is empty?
What problem does this PR solve?
handle a special case in GetRegion function
Issue Number: ref #6711
What is changed and how does it work?
The (item, index) returned by GetWithIndex(key) function has the following meaning:
Since the return values have different meanings in different situations, I want to unify them. To do this, I want to make sure item object is the largest interval (sorted by startkey) whose startkey <= key, and index is the number of interval object whose startkey <= key. Line 1794 - 1805 did this.
To find out if a key lies in an interval, we need to
Line 1807 - 1814 did this.
To find out if a region [k1, k2) does not overlap with any existing intervals, we need to
The 2) check is necessary in case we have an existing interval [2, 4), and we are querying [1, 5). Without the 2) check, it would return true. Line 1824 - 1826 did this.
Line 1816 - 1823 is handling a special case
Check List
Tests
Code changes
Side effects
Related changes
pingcap/docs
/pingcap/docs-cn
:pingcap/tiup
:Release note