fix(keyviz): find nearest id if not found #1268
Conversation
|
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1268 +/- ##
==========================================
+ Coverage 31.77% 31.94% +0.17%
==========================================
Files 310 310
Lines 17642 17712 +70
Branches 806 806
==========================================
+ Hits 5605 5658 +53
- Misses 11805 11821 +16
- Partials 232 233 +1
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
shhdgit
force-pushed
the
fix/keyviz-invalid-border
branch
from
6c8dcb6
to
52d47d4
Compare
shhdgit
force-pushed
the
fix/keyviz-invalid-border
branch
from
e99a6e6
to
c67db19
Compare
pkg/keyvisual/decorator/tidb.go
Outdated
| func (inOrder *tableInOrder) findOne(fromID, toID int64) *tableDetail { | ||
| if fromID >= toID { | ||
| return nil | ||
| } | ||
|
|
||
| inOrder.rwMu.RLock() | ||
| defer inOrder.rwMu.RUnlock() | ||
|
|
||
| tLen := len(inOrder.tables) | ||
| pivot := tLen / 2 | ||
| left := 0 | ||
| right := tLen | ||
| for pivot > left { | ||
| prevID := inOrder.tables[pivot-1].ID | ||
| id := inOrder.tables[pivot].ID | ||
| // find approaching id near the fromId | ||
| // table_1 ------- table_3 ------- table_5 | ||
| // ^ | ||
| // search table_2 | ||
| // approaching result: table_3 | ||
| if prevID < fromID && id >= fromID { | ||
| break | ||
| } | ||
|
|
||
| if id < fromID { | ||
| left = pivot | ||
| } else { | ||
| right = pivot | ||
| } | ||
| pivot = (left + right) / 2 | ||
| } | ||
|
|
||
| id := inOrder.tables[pivot].ID | ||
| if id < fromID || id >= toID { | ||
| return nil | ||
| } | ||
|
|
||
| return inOrder.tables[pivot] | ||
| } |
There was a problem hiding this comment.
How about simply use the binary search in std?
There was a problem hiding this comment.
Ok, I got it, what you want to do is not a simple binary search.
| // ^ | ||
| // search table_2 | ||
| // approaching result: table_3 | ||
| if prevID < fromID && id >= fromID { |
There was a problem hiding this comment.
Do you mean out of the range logic in L117-L120?😂
shhdgit
force-pushed
the
fix/keyviz-invalid-border
branch
from
93cd068
to
dd149e5
Compare
shhdgit
force-pushed
the
fix/keyviz-invalid-border
branch
from
dd149e5
to
52a6f22
Compare
|
Please fix the lint. Also I believe you need more tests to cover the "findOne" function. For such complicated logic function, maybe better to ensure 100% branch coverage. |
shhdgit
force-pushed
the
fix/keyviz-invalid-border
branch
from
59261b5
to
9cd2564
Compare
|
How about using sort.Search to discover a lower bound, and then check whether it meets upper bound? In this way you don't need to manually write the binary search or lower_bound algorithm. Here is an example of using sort.Search to implement lower_bound: https://go-review.googlesource.com/c/go/+/29333/3/src/sort/example_search_test.go#45 |
|
@shhdgit: PR needs rebase. 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. |
No description provided.