Fix: interval tree complete implemention based on red-black tree #10881

xkeyideal · 2019-07-09T07:29:58Z

Fixed interval tree

jingyih · 2019-07-09T20:48:16Z

pkg/adt/interval_tree.go

@@ -425,14 +475,14 @@ func (ivt *IntervalTree) find(ivl Interval) (ret *intervalNode) {
 		ret = n
 		return false
 	}
-	ivt.root.visit(&ivl, f)
+	ivt.root.visit(&ivl, ivt.nilNode, f)
 	return ret
 }

 // Find gets the IntervalValue for the node matching the given interval
 func (ivt *IntervalTree) Find(ivl Interval) (ret *IntervalValue) {
 	n := ivt.find(ivl)


If not found, does find() return nil or nilNode?

find() function return is nilNode, it is private function, I think its ok.

please help me check the code

codecov-io · 2019-07-11T08:59:58Z

Codecov Report

Merging #10881 into master will increase coverage by 1.72%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master   #10881      +/-   ##
==========================================
+ Coverage   62.14%   63.86%   +1.72%     
==========================================
  Files         398      401       +3     
  Lines       37483    37483              
==========================================
+ Hits        23293    23939     +646     
+ Misses      12597    11930     -667     
- Partials     1593     1614      +21

Impacted Files	Coverage Δ
proxy/grpcproxy/cache/store.go	`91.54% <100%> (+0.12%)`	⬆️
etcdserver/api/v3rpc/key.go	`80.28% <100%> (ø)`	⬆️
auth/range_perm_cache.go	`60.75% <100%> (ø)`	⬆️
mvcc/watcher_group.go	`89.7% <100%> (+0.07%)`	⬆️
pkg/adt/interval_tree.go	`93.02% <94.04%> (+9.23%)`	⬆️
client/client.go	`53.26% <0%> (-11.77%)`	⬇️
proxy/grpcproxy/watcher.go	`85.71% <0%> (-8.17%)`	⬇️
pkg/tlsutil/tlsutil.go	`86.2% <0%> (-6.9%)`	⬇️
raft/node.go	`89.47% <0%> (-4.21%)`	⬇️
raft/tracker/inflights.go	`91.83% <0%> (-3.72%)`	⬇️
... and 57 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 0af1697...d915387. Read the comment docs.

xkeyideal · 2019-07-12T01:51:03Z

@jingyih I had change the find function bugs, but Travis CI failed, please help see it.

jingyih · 2019-07-12T02:11:37Z

https://travis-ci.com/etcd-io/etcd/jobs/215119452
This failed due to formatting issue. Can you gofmt the file you modified?

https://travis-ci.com/etcd-io/etcd/jobs/215119455
This failed due to some known flaky tests. It is unrelated to this PR.

xkeyideal · 2019-07-12T03:11:18Z

@jingyih

Thank for your help.

This PR CI had passed.

xkeyideal · 2019-07-12T03:16:54Z

The interval tree code visit() will call a node visitor on each node that overlaps the given interval.

can your optimize the function performance

jingyih · 2019-07-12T19:00:11Z

@xiang90 PTAL.

xiang90 · 2019-07-29T23:42:46Z

.gitignore

@@ -34,3 +34,5 @@ vendor/**/*
 vendor/**/*_test.go

 *.bak
+
+.vscode/


xiang90 · 2019-07-29T23:43:54Z

pkg/adt/interval_tree.go

@@ -171,45 +171,60 @@ type IntervalValue struct {
 type IntervalTree struct {
 	root  *intervalNode
 	count int
+
+	// red-black NIL node, can not use golang nil instead


can you explain why we cannot use golang nil to represent the NIL node? If we do need a NIL node instance for comparison, shall we make it a package global variable so that we do not need to pass it around?

@xiang90 See the issue #10877 , when delete node 11， if use golang nil instead of rbtree NIL node, the tree is not rbtree.

Because of delete node 11, the NIL node should be coloring double black, but use golang nil will nothing to do.

@xiang90
Because of the tree is also Binary Search Tree, so the code all cases are right.

The Introduction to algorithms tell us when delete leaf black node, then NIL node will be double black.

Red-black tree nature 5: For each node, the same number of black nodes are included on the path of the node nodes after reaching the node.

@xiang90 The NIL node can not a package global variable. It's a pointer variable, there are many trees.

gyuho · 2019-07-30T00:43:03Z

@xkeyideal @xiang90 @jingyih I am writing a test case to validate the fix. Will cherry-pick this patch with some clean up + documentation.

xiang90 · 2019-07-30T01:51:31Z

pkg/adt/interval_tree.go

@@ -353,45 +367,54 @@ func (ivt *IntervalTree) insertFixup(z *intervalNode) {

 // rotateLeft moves x so it is left of its right child
 func (ivt *IntervalTree) rotateLeft(x *intervalNode) {
+


remove this empty line

xiang90 · 2019-07-30T01:51:42Z

pkg/adt/interval_tree.go

 func (ivt *IntervalTree) rotateRight(x *intervalNode) {
-	if x == nil {
+


xiang90 · 2019-07-30T01:51:57Z

pkg/adt/interval_tree.go

-		if n.iv.Ivl != ivl {
-			return true
+func (ivt *IntervalTree) find(ivl Interval) *intervalNode {
+


xiang90 · 2019-07-30T01:53:54Z

pkg/adt/interval_tree.go

 	f := func(n *IntervalValue) bool {
 		ivt.Insert(n.Ivl, n.Val)
 		return true
 	}
 	inIvt.Visit(ivl, f)
 }

+func (ivt *IntervalTree) LevelOrder() [][]*intervalNode {


is this only used by test? if so, move this to test file.

Yes, I will change.

xiang90 · 2019-07-30T01:54:00Z

pkg/adt/interval_tree.go

 	f := func(n *IntervalValue) bool {
 		ivt.Insert(n.Ivl, n.Val)
 		return true
 	}
 	inIvt.Visit(ivl, f)
 }

+func (ivt *IntervalTree) LevelOrder() [][]*intervalNode {
+


remove empty line

xiang90 · 2019-07-30T01:54:53Z

pkg/adt/interval_tree_test.go

+		{&intervalNode{iv: IntervalValue{NewInt64Interval(953, 954), 12}}, red},
+	}
+
+	/*


use go style comment. do not use /*, use // instead.

xiang90 · 2019-07-30T02:15:36Z

pkg/adt/interval_tree.go

 	}
-	ivt.root.visit(&ivl, f)


why not just call visit as before?

the visit function maybe visit all nodes for find one node, the new find function use BST nature can prune, but when find left interval same, the must use visit function visit the subtree all nodes for find the node

if this is an optimization, can we move this to a separate PR?

I think it is just make it better, not optimization

OK. Then can we not include this enhancement in this PR?

OK, the change the find function just use visit

xkeyideal · 2019-07-30T02:49:38Z

@xiang90 Please help me see the Travis CI why not build failed.

gyuho · 2019-07-31T01:26:34Z

This doesn't seem to fix the issue. Let me rework on this.

gyuho · 2019-08-01T20:02:53Z

@xkeyideal I tried your patch with sentinel node. It fixes the delete operation maintaining the black-height properties, but it breaks visit operation thus breaking contains and stab methods. Could you take a look #10965 and try again with latest changes? I refactored the code with failure test cases from this PR. Thanks!

xkey added 2 commits July 9, 2019 15:28

fix: interval tree complete implemention based on red-black tree

e9d204d

fix: interval tree

dd7a62d

jingyih reviewed Jul 9, 2019

View reviewed changes

fix: interval tree

c62f5b7

xkey added 2 commits July 11, 2019 18:06

fix: interval tree find the exact node same Begin not found bugs

a2d0475

fix: find exact node

16a17bb

fix: gofmt checking failed: pkg/adt/interval_tree_test.go

f1218d7

gyuho mentioned this pull request Jul 26, 2019

Release etcd 3.4 #10943

Closed

19 tasks

xiang90 reviewed Jul 29, 2019

View reviewed changes

.gitignore

@@ -34,3 +34,5 @@ vendor/**/*

vendor/**/*_test.go

*.bak

.vscode/

Copy link

Contributor

xiang90 Jul 29, 2019

Choose a reason for hiding this comment

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

new line

xiang90 reviewed Jul 29, 2019

View reviewed changes

gyuho self-assigned this Jul 30, 2019

xiang90 reviewed Jul 30, 2019

View reviewed changes

pkg/adt/interval_tree.go Outdated

func (ivt *IntervalTree) rotateRight(x *intervalNode) {

if x == nil {

Copy link

Contributor

xiang90 Jul 30, 2019

Choose a reason for hiding this comment

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

remove

xiang90 reviewed Jul 30, 2019

View reviewed changes

fix: pr code use go style

d736a7b

xiang90 reviewed Jul 30, 2019

View reviewed changes

fix: change the find function use visit

d915387

gyuho mentioned this pull request Aug 1, 2019

[TESTING xkeyideal] pkg/adt: fix red black tree to maintain black-height property #10963

Closed

gyuho mentioned this pull request Aug 1, 2019

[TESTING textbook implementation] pkg/adt: fix red black tree to maintain black-height property #10964

Closed

gyuho closed this Aug 1, 2019

liyue201 mentioned this pull request Nov 7, 2019

some questions about the implementation of intervalTree #11337

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: interval tree complete implemention based on red-black tree #10881

Fix: interval tree complete implemention based on red-black tree #10881

xkeyideal commented Jul 9, 2019

jingyih Jul 9, 2019

xkeyideal Jul 9, 2019

codecov-io commented Jul 11, 2019 •

edited

Loading

xkeyideal commented Jul 12, 2019

jingyih commented Jul 12, 2019

xkeyideal commented Jul 12, 2019

xkeyideal commented Jul 12, 2019

jingyih commented Jul 12, 2019

xiang90 Jul 29, 2019

xiang90 Jul 29, 2019

xkeyideal Jul 30, 2019

xkeyideal Jul 30, 2019

xkeyideal Jul 30, 2019

gyuho commented Jul 30, 2019

xiang90 Jul 30, 2019

xiang90 Jul 30, 2019

xiang90 Jul 30, 2019

xiang90 Jul 30, 2019

xkeyideal Jul 30, 2019

xiang90 Jul 30, 2019

xiang90 Jul 30, 2019

xiang90 Jul 30, 2019

xkeyideal Jul 30, 2019

xiang90 Jul 30, 2019

xkeyideal Jul 30, 2019

xiang90 Jul 30, 2019

xkeyideal Jul 30, 2019

xkeyideal commented Jul 30, 2019

gyuho commented Jul 31, 2019

gyuho commented Aug 1, 2019

		@@ -353,45 +367,54 @@ func (ivt IntervalTree) insertFixup(z intervalNode) {

		// rotateLeft moves x so it is left of its right child
		func (ivt IntervalTree) rotateLeft(x intervalNode) {

		func (ivt IntervalTree) rotateRight(x intervalNode) {
		if x == nil {

Fix: interval tree complete implemention based on red-black tree #10881

Fix: interval tree complete implemention based on red-black tree #10881

Conversation

xkeyideal commented Jul 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 11, 2019 • edited Loading

Codecov Report

xkeyideal commented Jul 12, 2019

jingyih commented Jul 12, 2019

xkeyideal commented Jul 12, 2019

xkeyideal commented Jul 12, 2019

jingyih commented Jul 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Jul 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xkeyideal commented Jul 30, 2019

gyuho commented Jul 31, 2019

gyuho commented Aug 1, 2019

codecov-io commented Jul 11, 2019 •

edited

Loading