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

pkg/adt: refactor + add more test cases #10959

Merged
merged 7 commits into from
Aug 1, 2019
Merged

pkg/adt: refactor + add more test cases #10959

merged 7 commits into from
Aug 1, 2019

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Jul 30, 2019

Introduction to Algorithms by Charles E. Leiserson, Clifford Stein, Ronald Rivest, and Thomas H. Cormen:

13.1 Properties of red-black trees

  1. For each node, all simple paths from the node to descendant leaves contain the
    same number of black nodes.

Current delete operation violates "black-height" property.


For example, given this red-black tree:

red-black-tree-02-delete-514

Let's delete the node 11.

The tree should be rebalanced as below:

After deleting 11 (requires rebalancing):

                        [510,511]
                        /      \
                --------        ----------------------------
               /                                            \
           [383,384]                                     [830,831]
           /       \                                    /          \
          /         \                                  /            \
[261,262](red)     [410,411]                  [647,648]           [899,900](red)
        /               \                           \                     /    \
       /                 \                           \                   /      \
    [82,83]           [292,293]                [815,816](red)      [888,889]    [972,973]
        \                                                           /
         \                                                         /
        [238,239](red)                                       [953,954](red)

red-black-tree-09-delete-11

However, current implementation returns:

After deleting 11 (requires rebalancing):

                       [510,511]
                        /      \
              ----------        --------------------------
             /                                            \
         [82,83]                                       [830,831]
               \                                      /          \
                \                                    /            \
             [383,384]                        [647,648]           [899,900]
             /       \                              \                 /    \
            /         \                              \               /      \
      [261,262]      [410,411]                      [815,816] [888,889]    [972,973]
        /   \                                                                /
       /     \                                                              /
[238,239]   [292,293]                                                [953,954]

This violates "black-height" property.

Fix #10877.

@gyuho gyuho added the WIP label Jul 30, 2019
@gyuho gyuho added this to the etcd-v3.4 milestone Jul 30, 2019
@gyuho
Copy link
Contributor Author

gyuho commented Jul 30, 2019

@xiang90 @xkeyideal This is still in progress. Still validating @xkeyideal's fix.

Current branch includes a test case to prove that the current implementation is wrong, as described in #10877.

@gyuho gyuho mentioned this pull request Jul 30, 2019
19 tasks
Make "IntervalTree" an interface to abstract range tree interface

Signed-off-by: Gyuho Lee <[email protected]>
@xkeyideal
Copy link

@gyuho I saw your code, it seem delete function doesn't be modified, nil node should be coloring black, then delete node 11, the nil node can be coloring double black

@gyuho
Copy link
Contributor Author

gyuho commented Jul 31, 2019

@xkeyideal Yes I am still working on this. I created this PR just to validate your test case.

gyuho added 4 commits July 31, 2019 10:05
Described in etcd-io#10877.

"black-height" property: Every path from a node to any descendant leaf node must have the same number of black nodes.

Expected

    After deleting 11 (requires rebalancing):
                            [510,511]
                             /      \
                   ----------        --------------------------
                  /                                            \
              [383,384]                                       [830,831]
              /       \                                      /          \
             /         \                                    /            \
      [261,262](red)  [410,411]                     [647,648]           [899,900](red)
          /               \                              \                      /    \
         /                 \                              \                    /      \
      [82,83]           [292,293]                      [815,816](red)   [888,889]    [972,973]
            \                                                           /
             \                                                         /
          [238,239](red)                                       [953,954](red)

Got

    After deleting 11 (requires rebalancing):
                            [510,511]
                             /      \
                   ----------        --------------------------
                  /                                            \
              [82,83]                                       [830,831]
                    \                                      /          \
                     \                                    /            \
                  [383,384]                        [647,648]            [899,900]
                  /       \                              \                  /    \
                 /         \                              \                /      \
           [261,262]      [410,411]                      [815,816]   [888,889]    [972,973]
             /   \                                                                  /
            /     \                                                                /
     [238,239]   [292,293]                                                  [953,954]

This violates "black-height" property.

Signed-off-by: Gyuho Lee <[email protected]>
@jingyih
Copy link
Contributor

jingyih commented Aug 1, 2019

Is the bug about correctness or just performance?

@gyuho gyuho removed the WIP label Aug 1, 2019
@gyuho gyuho changed the title pkg/adt: fix delete operation pkg/adt: refactor + add more test cases Aug 1, 2019
@gyuho
Copy link
Contributor Author

gyuho commented Aug 1, 2019

@jingyih

Can you help merge this? This doesn't change any behavior with current implementation.

I created a separate issue to track this.

I think we need more time to work on this, in 3.5.

@codecov-io
Copy link

Codecov Report

Merging #10959 into master will decrease coverage by 0.32%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10959      +/-   ##
==========================================
- Coverage   64.21%   63.88%   -0.33%     
==========================================
  Files         401      401              
  Lines       37473    37521      +48     
==========================================
- Hits        24062    23970      -92     
- Misses      11804    11936     +132     
- Partials     1607     1615       +8
Impacted Files Coverage Δ
auth/range_perm_cache.go 59.49% <100%> (-1.27%) ⬇️
proxy/grpcproxy/cache/store.go 91.54% <100%> (+0.12%) ⬆️
etcdserver/api/v3rpc/key.go 80.28% <100%> (ø) ⬆️
mvcc/watcher_group.go 89.7% <100%> (+0.07%) ⬆️
pkg/adt/interval_tree.go 86.47% <90.16%> (-5.12%) ⬇️
auth/store.go 48.4% <0%> (-17.5%) ⬇️
etcdserver/api/v3rpc/lease.go 67.04% <0%> (-7.96%) ⬇️
pkg/tlsutil/tlsutil.go 86.2% <0%> (-6.9%) ⬇️
clientv3/namespace/watch.go 87.87% <0%> (-6.07%) ⬇️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 149e5dc...6a0811a. Read the comment docs.

@jingyih
Copy link
Contributor

jingyih commented Aug 1, 2019

LGTM. Thanks! Let's work on this in v3.5.

@jingyih jingyih merged commit 456c91b into etcd-io:master Aug 1, 2019
@gyuho gyuho deleted the adt branch August 1, 2019 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

The interval tree is not complete implementation based on red-black tree
4 participants