Merge pull request #251 from apelisse/add-compare-duplicates

typed: Update compare algorithm to handle duplicates

Author: k8s-ci-robot

Diff for: fieldpath/pathelementmap.go

@@ -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
@@ -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)
 	})
@@ -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)
 	})

Diff for: typed/compare.go

@@ -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)
@@ -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)
@@ -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

Diff for: typed/symdiff_test.go

@@ -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":[]}`,
@@ -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",
@@ -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")),
 	}},
 }}
 
@@ -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)
 			}