Skip to content
This repository was archived by the owner on Aug 2, 2021. It is now read-only.

swarm/pot: pot.remove fixed #1098

Closed
wants to merge 2 commits into from
Closed

swarm/pot: pot.remove fixed #1098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
357d536
swarm/pot: refactored pot.remove(), updated comments
gluk256 Jan 8, 2019
dbfd8ec
swarm/pot: comments updated
gluk256 Jan 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion swarm/pot/address.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ func ToBytes(v Val) []byte {
}

// DefaultPof returns a proximity order comparison operator function
// where all
func DefaultPof(max int) func(one, other Val, pos int) (int, bool) {
return func(one, other Val, pos int) (int, bool) {
po, eq := proximityOrder(ToBytes(one), ToBytes(other), pos)
Expand All @@ -174,6 +173,9 @@ func DefaultPof(max int) func(one, other Val, pos int) (int, bool) {
}
}

// proximityOrder returns two parameters:
// 1. relative proximity order of the arguments one & other;
// 2. boolean indicating whether the full match occurred (one == other).
func proximityOrder(one, other []byte, pos int) (int, bool) {
for i := pos / 8; i < len(one); i++ {
if one[i] == other[i] {
Expand Down
18 changes: 6 additions & 12 deletions swarm/pot/pot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,10 @@ func add(t *Pot, val Val, pof Pof) (*Pot, int, bool) {
return r, po, found
}

// Remove called on (v) deletes v from the Pot and returns
// the proximity order of v and a boolean value indicating
// if the value was found
// Remove called on (t, v) returns a new Pot that contains all the elements of t
// minus the value v, using the applicative remove
// the second return value is the proximity order of the inserted element
// the third is boolean indicating if the item was found
// Remove deletes element v from the Pot t and returns three parameters:
// 1. new Pot that contains all the elements of t minus the element v;
// 2. proximity order of the removed element v;
// 3. boolean indicating whether the item was found.
func Remove(t *Pot, v Val, pof Pof) (*Pot, int, bool) {
return remove(t, v, pof)
}
Expand All @@ -161,10 +158,7 @@ func remove(t *Pot, val Val, pof Pof) (r *Pot, po int, found bool) {
if found {
size--
if size == 0 {
r = &Pot{
po: t.po,
}
return r, po, true
return &Pot{}, po, true
}
i := len(t.bins) - 1
last := t.bins[i]
Expand Down Expand Up @@ -201,7 +195,7 @@ func remove(t *Pot, val Val, pof Pof) (r *Pot, po int, found bool) {
}
bins = append(bins, t.bins[j:]...)
r = &Pot{
Copy link
Contributor

Choose a reason for hiding this comment

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

why not return the same pot if not found?

Copy link
Author

Choose a reason for hiding this comment

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

because the function is recursive, so if not found at the current level of pot tree, it might be be found deeper.

Copy link
Contributor

Choose a reason for hiding this comment

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

But if not found the contents of the newly created Pot object is identical to the one passed, no? Is the intention that the Pot object that is passed to this (and similar) functions is discarded (only use the returned Pot)?

pin: val,
pin: t.pin,
Copy link
Contributor

Choose a reason for hiding this comment

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

wow!

size: size,
po: t.po,
bins: bins,
Expand Down
84 changes: 77 additions & 7 deletions swarm/pot/pot_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,72 @@ func testAdd(t *Pot, pof Pof, j int, values ...string) (_ *Pot, n int, f bool) {
return t, n, f
}

// removing non-existing element from pot
func TestPotRemoveNonExisting(t *testing.T) {
pof := DefaultPof(8)
n := NewPot(newTestAddr("00111100", 0), 0)
n, _, _ = Remove(n, newTestAddr("00000101", 0), pof)
exp := "00111100"
got := Label(n.Pin())
if got[:8] != exp {
t.Fatalf("incorrect pinned value. Expected %v, got %v", exp, got[:8])
}
}

// this test creates hierarchical pot tree, and therefore any child node will have
// child_po = parent_po + 1.
// then removes a node from the middle of the tree.
func TestPotRemoveSameBin(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain what the tests are doing in a comment

pof := DefaultPof(8)
n := NewPot(newTestAddr("11111111", 0), 0)
n, _, _ = testAdd(n, pof, 1, "00000000", "01000000", "01100000", "01110000", "01111000")
n, _, _ = Remove(n, newTestAddr("01110000", 0), pof)
inds, po := indexes(n)
goti := n.Size()
expi := 5
if goti != expi {
t.Fatalf("incorrect number of elements in Pot. Expected %v, got %v", expi, goti)
}
inds, po = indexes(n)
got := fmt.Sprintf("%v", inds)
exp := "[5 3 2 1 0]"
if got != exp {
t.Fatalf("incorrect indexes in iteration over Pot. Expected %v, got %v", exp, got)
}
got = fmt.Sprintf("%v", po)
exp = "[3 2 1 0 0]"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why these po's are correct.

If the pin has its highest-order bit set to 1, shouldn't any address with highest-order bit 0 mean po == 0?

Following the logic of this expectation, shouldn't it be rather [5 3 2 0]?

Why are there 5 elements after deleting one? Does that mean we count the pin aswell? If so, what does the po actually mean here? Is it po in relation to the pin? And if it is, why isn't there a po == 256 first, which would be the pin address compared to itself?

Copy link
Author

Choose a reason for hiding this comment

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

po is calculated in relation to the parent node in the pot

Copy link
Contributor

Choose a reason for hiding this comment

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

Your answer doesn't clear up my questions, I'm afraid.

What is "parent node." Is it the same as the pin?

if got != exp {
t.Fatalf("incorrect po-s in iteration over Pot. Expected %v, got %v", exp, got)
}
}

// this test creates a flat pot tree (all the elements are leafs of one root),
// and therefore they all have the same po.
// then removes an arbitrary element from the pot.
func TestPotRemoveDifferentBins(t *testing.T) {
pof := DefaultPof(8)
n := NewPot(newTestAddr("11111111", 0), 0)
n, _, _ = testAdd(n, pof, 1, "00000000", "10000000", "11000000", "11100000", "11110000")
n, _, _ = Remove(n, newTestAddr("11100000", 0), pof)
inds, po := indexes(n)
goti := n.Size()
expi := 5
if goti != expi {
t.Fatalf("incorrect number of elements in Pot. Expected %v, got %v", expi, goti)
}
inds, po = indexes(n)
got := fmt.Sprintf("%v", inds)
exp := "[1 2 3 5 0]"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the 0 last?

Copy link
Author

Choose a reason for hiding this comment

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

this is how this test function indexes() works. otherwise it's irrelevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate, please? Are you saying that the order doesn't matter? Why doesn't the order matter?

if got != exp {
t.Fatalf("incorrect indexes in iteration over Pot. Expected %v, got %v", exp, got)
}
got = fmt.Sprintf("%v", po)
exp = "[0 1 2 4 0]"
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the last 0 represent here?

Copy link
Author

Choose a reason for hiding this comment

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

the root

Copy link
Contributor

@nolash nolash Jan 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

So we have "root," "parent node," and "pin." Are they the same thing? Please let's use one term.

Grep'ing the files doesn't really clear up the terms either:

$ grep -i "parent" swarm/pot/ -r
swarm/pot/pot_test.go:// child_po = parent_po + 1.
$ grep -i "root" swarm/pot/ -r
swarm/pot/pot_test.go:// 	log.Root().SetHandler(log.LvlFilterHandler(log.LvlTrace, log.StreamHandler(os.Stderr, log.TerminalFormat(false))))
swarm/pot/pot_test.go:// this test creates a flat pot tree (all the elements are leafs of one root),
swarm/pot/pot.go:// Pot is the node type (same for root, branching node and leaf)
swarm/pot/pot.go:// the function argument is passed the value and the proximity order wrt the root pin
swarm/pot/pot.go:// it does NOT include the pinned item of the root

"root pin" .. "pinned item of root" ...

if got != exp {
t.Fatalf("incorrect po-s in iteration over Pot. Expected %v, got %v", exp, got)
}
}

func TestPotAdd(t *testing.T) {
pof := DefaultPof(8)
n := NewPot(newTestAddr("00111100", 0), 0)
Expand Down Expand Up @@ -135,25 +201,29 @@ func TestPotRemove(t *testing.T) {
t.Fatalf("incorrect number of elements in Pot. Expected %v, got %v", expi, goti)
}
inds, po := indexes(n)
got = fmt.Sprintf("%v", po)
exp = "[1 3 0]"
if got != exp {
t.Fatalf("incorrect po-s in iteration over Pot. Expected %v, got %v", exp, got)
}
got = fmt.Sprintf("%v", inds)
exp = "[2 4 0]"
exp = "[2 4 1]"
if got != exp {
t.Fatalf("incorrect indexes in iteration over Pot. Expected %v, got %v", exp, got)
}
got = fmt.Sprintf("%v", po)
exp = "[1 3 0]"
n, _, _ = Remove(n, newTestAddr("00111100", 0), pof) // remove again same element
inds, _ = indexes(n)
got = fmt.Sprintf("%v", inds)
if got != exp {
t.Fatalf("incorrect po-s in iteration over Pot. Expected %v, got %v", exp, got)
t.Fatalf("incorrect indexes in iteration over Pot. Expected %v, got %v", exp, got)
}
// remove again
n, _, _ = Remove(n, newTestAddr("00111100", 0), pof)
n, _, _ = Remove(n, newTestAddr("00000000", 0), pof) // remove the first element
inds, _ = indexes(n)
got = fmt.Sprintf("%v", inds)
exp = "[2 4]"
if got != exp {
t.Fatalf("incorrect indexes in iteration over Pot. Expected %v, got %v", exp, got)
}

}

func TestPotSwap(t *testing.T) {
Expand Down