Skip to content

Commit

Permalink
Merge pull request #251 from apelisse/add-compare-duplicates
Browse files Browse the repository at this point in the history 
typed: Update compare algorithm to handle duplicates
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Oct 12, 2023
2 parents 66711bf + a1f0e95 commit 1233172
Show file tree
Hide file tree
Showing 3 changed files with 202 additions and 36 deletions.
43 changes: 35 additions & 8 deletions fieldpath/pathelementmap.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,15 @@ import (
// for PathElementSet and SetNodeMap, so we could probably share the
// code.
type PathElementValueMap struct {
members sortedPathElementValues
valueMap PathElementMap
}

func MakePathElementValueMap(size int) PathElementValueMap {
return PathElementValueMap{
members: make(sortedPathElementValues, 0, size),
valueMap: MakePathElementMap(size),
}
}

type pathElementValue struct {
PathElement PathElement
Value value.Value
}

type sortedPathElementValues []pathElementValue

// Implement the sort interface; this would permit bulk creation, which would
Expand All @@ -55,6 +50,38 @@ func (spev sortedPathElementValues) Swap(i, j int) { spev[i], spev[j] = spev[j],
// Insert adds the pathelement and associated value in the map.
// If insert is called twice with the same PathElement, the value is replaced.
func (s *PathElementValueMap) Insert(pe PathElement, v value.Value) {
s.valueMap.Insert(pe, v)
}

// Get retrieves the value associated with the given PathElement from the map.
// (nil, false) is returned if there is no such PathElement.
func (s *PathElementValueMap) Get(pe PathElement) (value.Value, bool) {
v, ok := s.valueMap.Get(pe)
if !ok {
return nil, false
}
return v.(value.Value), true
}

// PathElementValueMap is a map from PathElement to interface{}.
type PathElementMap struct {
members sortedPathElementValues
}

type pathElementValue struct {
PathElement PathElement
Value interface{}
}

func MakePathElementMap(size int) PathElementMap {
return PathElementMap{
members: make(sortedPathElementValues, 0, size),
}
}

// Insert adds the pathelement and associated value in the map.
// If insert is called twice with the same PathElement, the value is replaced.
func (s *PathElementMap) Insert(pe PathElement, v interface{}) {
loc := sort.Search(len(s.members), func(i int) bool {
return !s.members[i].PathElement.Less(pe)
})
Expand All @@ -73,7 +100,7 @@ func (s *PathElementValueMap) Insert(pe PathElement, v value.Value) {

// Get retrieves the value associated with the given PathElement from the map.
// (nil, false) is returned if there is no such PathElement.
func (s *PathElementValueMap) Get(pe PathElement) (value.Value, bool) {
func (s *PathElementMap) Get(pe PathElement) (interface{}, bool) {
loc := sort.Search(len(s.members), func(i int) bool {
return !s.members[i].PathElement.Less(pe)
})
Expand Down
109 changes: 83 additions & 26 deletions typed/compare.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,16 @@ func (w *compareWalker) visitListItems(t *schema.List, lhs, rhs value.List) (err
lLen = lhs.Length()
}

lValues := fieldpath.MakePathElementValueMap(lLen)
maxLength := rLen
if lLen > maxLength {
maxLength = lLen
}
// Contains all the unique PEs between lhs and rhs, exactly once.
// Order doesn't matter since we're just tracking ownership in a set.
allPEs := make([]fieldpath.PathElement, 0, maxLength)

// Gather all the elements from lhs, indexed by PE, in a list for duplicates.
lValues := fieldpath.MakePathElementMap(lLen)
for i := 0; i < lLen; i++ {
child := lhs.At(i)
pe, err := listItemToPathElement(w.allocator, w.schema, t, child)
Expand All @@ -224,14 +233,18 @@ func (w *compareWalker) visitListItems(t *schema.List, lhs, rhs value.List) (err
// this element.
continue
}
// Ignore repeated occurences of `pe`.
if _, found := lValues.Get(pe); found {
continue

if v, found := lValues.Get(pe); found {
list := v.([]value.Value)
lValues.Insert(pe, append(list, child))
} else {
lValues.Insert(pe, []value.Value{child})
allPEs = append(allPEs, pe)
}
lValues.Insert(pe, child)
}

rValues := fieldpath.MakePathElementValueMap(rLen)
// Gather all the elements from rhs, indexed by PE, in a list for duplicates.
rValues := fieldpath.MakePathElementMap(rLen)
for i := 0; i < rLen; i++ {
rValue := rhs.At(i)
pe, err := listItemToPathElement(w.allocator, w.schema, t, rValue)
Expand All @@ -242,31 +255,75 @@ func (w *compareWalker) visitListItems(t *schema.List, lhs, rhs value.List) (err
// this element.
continue
}
// Ignore repeated occurences of `pe`.
if _, found := rValues.Get(pe); found {
continue
if v, found := rValues.Get(pe); found {
list := v.([]value.Value)
rValues.Insert(pe, append(list, rValue))
} else {
rValues.Insert(pe, []value.Value{rValue})
if _, found := lValues.Get(pe); !found {
allPEs = append(allPEs, pe)
}
}
rValues.Insert(pe, rValue)
// We can compare with nil if lValue is not present.
lValue, _ := lValues.Get(pe)
errs = append(errs, w.compareListItem(t, pe, lValue, rValue)...)
}

// Add items from left that are not in right.
for i := 0; i < lLen; i++ {
lValue := lhs.At(i)
pe, err := listItemToPathElement(w.allocator, w.schema, t, lValue)
if err != nil {
errs = append(errs, errorf("element %v: %v", i, err.Error())...)
// If we can't construct the path element, we can't
// even report errors deeper in the schema, so bail on
// this element.
continue
for _, pe := range allPEs {
lList := []value.Value(nil)
if l, ok := lValues.Get(pe); ok {
lList = l.([]value.Value)
}
if _, found := rValues.Get(pe); found {
continue
rList := []value.Value(nil)
if l, ok := rValues.Get(pe); ok {
rList = l.([]value.Value)
}

switch {
case len(lList) == 0 && len(rList) == 0:
// We shouldn't be here anyway.
return
// Normal use-case:
// We have no duplicates for this PE, compare items one-to-one.
case len(lList) <= 1 && len(rList) <= 1:
lValue := value.Value(nil)
if len(lList) != 0 {
lValue = lList[0]
}
rValue := value.Value(nil)
if len(rList) != 0 {
rValue = rList[0]
}
errs = append(errs, w.compareListItem(t, pe, lValue, rValue)...)
// Duplicates before & after use-case:
// Compare the duplicates lists as if they were atomic, mark modified if they changed.
case len(lList) >= 2 && len(rList) >= 2:
listEqual := func(lList, rList []value.Value) bool {
if len(lList) != len(rList) {
return false
}
for i := range lList {
if !value.Equals(lList[i], rList[i]) {
return false
}
}
return true
}
if !listEqual(lList, rList) {
w.comparison.Modified.Insert(append(w.path, pe))
}
// Duplicates before & not anymore use-case:
// Rcursively add new non-duplicate items, Remove duplicate marker,
case len(lList) >= 2:
if len(rList) != 0 {
errs = append(errs, w.compareListItem(t, pe, nil, rList[0])...)
}
w.comparison.Removed.Insert(append(w.path, pe))
// New duplicates use-case:
// Recursively remove old non-duplicate items, add duplicate marker.
case len(rList) >= 2:
if len(lList) != 0 {
errs = append(errs, w.compareListItem(t, pe, lList[0], nil)...)
}
w.comparison.Added.Insert(append(w.path, pe))
}
errs = append(errs, w.compareListItem(t, pe, lValue, nil)...)
}

return
Expand Down
86 changes: 84 additions & 2 deletions typed/symdiff_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,24 @@ var symdiffCases = []symdiffTestCase{{
removed: _NS(),
modified: _NS(),
added: _NS(_P("setStr", _V("c"))),
}, {
lhs: `{"setStr":["a"]}`,
rhs: `{"setStr":["a","b","b"]}`,
removed: _NS(),
modified: _NS(),
added: _NS(_P("setStr", _V("b"))),
}, {
lhs: `{"setStr":["a","b"]}`,
rhs: `{"setStr":["a","b","b"]}`,
removed: _NS(_P("setStr", _V("b"))),
modified: _NS(),
added: _NS(_P("setStr", _V("b"))),
}, {
lhs: `{"setStr":["b","b"]}`,
rhs: `{"setStr":["a","b","b"]}`,
removed: _NS(),
modified: _NS(),
added: _NS(_P("setStr", _V("a"))),
}, {
lhs: `{"setStr":["a","b","c"]}`,
rhs: `{"setStr":[]}`,
Expand All @@ -618,6 +636,28 @@ var symdiffCases = []symdiffTestCase{{
removed: _NS(_P("setNumeric", _V(3.14159))),
modified: _NS(),
added: _NS(_P("setNumeric", _V(3))),
}, {
lhs: `{"setStr":["a","b","b","c","a"]}`,
rhs: `{"setStr":[]}`,
removed: _NS(
_P("setStr", _V("a")),
_P("setStr", _V("b")),
_P("setStr", _V("c")),
),
modified: _NS(),
added: _NS(),
}, {
lhs: `{"setBool":[true,true]}`,
rhs: `{"setBool":[false]}`,
removed: _NS(_P("setBool", _V(true))),
modified: _NS(),
added: _NS(_P("setBool", _V(false))),
}, {
lhs: `{"setNumeric":[1,2,2,3.14159,1]}`,
rhs: `{"setNumeric":[1,2,3]}`,
removed: _NS(_P("setNumeric", _V(1)), _P("setNumeric", _V(2)), _P("setNumeric", _V(3.14159))),
modified: _NS(),
added: _NS(_P("setNumeric", _V(1)), _P("setNumeric", _V(2)), _P("setNumeric", _V(3))),
}},
}, {
name: "associative list",
Expand Down Expand Up @@ -743,6 +783,48 @@ var symdiffCases = []symdiffTestCase{{
removed: _NS(),
modified: _NS(_P("atomicList")),
added: _NS(),
}, {
lhs: `{"list":[{"key":"a","id":1,"nv":2},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
rhs: `{"list":[{"key":"a","id":1},{"key":"a","id":2}]}`,
removed: _NS(
_P("list", _KBF("key", "b", "id", 1)),
_P("list", _KBF("key", "b", "id", 1), "key"),
_P("list", _KBF("key", "b", "id", 1), "id"),
_P("list", _KBF("key", "a", "id", 1)),
),
modified: _NS(),
added: _NS(
_P("list", _KBF("key", "a", "id", 1)),
_P("list", _KBF("key", "a", "id", 1), "key"),
_P("list", _KBF("key", "a", "id", 1), "id"),
_P("list", _KBF("key", "a", "id", 2)),
_P("list", _KBF("key", "a", "id", 2), "key"),
_P("list", _KBF("key", "a", "id", 2), "id"),
),
}, {
lhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
rhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true,"nv":1}]}`,
removed: _NS(),
modified: _NS(_P("list", _KBF("key", "a", "id", 1))),
added: _NS(),
}, {
lhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
rhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
removed: _NS(),
modified: _NS(),
added: _NS(),
}, {
lhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
rhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1,"bv":true},{"key":"a","id":1,"bv":true}]}`,
removed: _NS(),
modified: _NS(),
added: _NS(_P("list", _KBF("key", "b", "id", 1), "bv")),
}, {
lhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}]}`,
rhs: `{"list":[{"key":"a","id":1},{"key":"b","id":1},{"key":"a","id":1,"bv":true}],"atomicList":["unrelated"]}`,
removed: _NS(),
modified: _NS(),
added: _NS(_P("atomicList")),
}},
}}

Expand All @@ -757,11 +839,11 @@ func (tt symdiffTestCase) test(t *testing.T) {
t.Parallel()
pt := parser.Type(tt.rootTypeName)

tvLHS, err := pt.FromYAML(quint.lhs)
tvLHS, err := pt.FromYAML(quint.lhs, typed.AllowDuplicates)
if err != nil {
t.Fatalf("failed to parse lhs: %v", err)
}
tvRHS, err := pt.FromYAML(quint.rhs)
tvRHS, err := pt.FromYAML(quint.rhs, typed.AllowDuplicates)
if err != nil {
t.Fatalf("failed to parse rhs: %v", err)
}
Expand Down

0 comments on commit 1233172

Please sign in to comment.