Skip to content

Commit

Permalink
internal: remove unused TrimDuplicates
Browse files Browse the repository at this point in the history 
Because the "internal" package is inaccessible for consumers of the klog/v2
API, removing code from it is okay.
  • Loading branch information
@pohly
pohly committed May 10, 2022
1 parent 42bf7a2 commit eb21886
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 122 deletions.
49 changes: 0 additions & 49 deletions internal/serialize/keyvalues.go
View file
Expand Up @@ -44,55 +44,6 @@ func WithValues(oldKV, newKV []interface{}) []interface{} {
return kv
}

// TrimDuplicates deduplicates elements provided in multiple key/value tuple
// slices, whilst maintaining the distinction between where the items are
// contained.
func TrimDuplicates(kvLists ...[]interface{}) [][]interface{} {
// maintain a map of all seen keys
seenKeys := map[interface{}]struct{}{}
// build the same number of output slices as inputs
outs := make([][]interface{}, len(kvLists))
// iterate over the input slices backwards, as 'later' kv specifications
// of the same key will take precedence over earlier ones
for i := len(kvLists) - 1; i >= 0; i-- {
// initialise this output slice
outs[i] = []interface{}{}
// obtain a reference to the kvList we are processing
// and make sure it has an even number of entries
kvList := kvLists[i]
if len(kvList)%2 != 0 {
kvList = append(kvList, missingValue)
}

// start iterating at len(kvList) - 2 (i.e. the 2nd last item) for
// slices that have an even number of elements.
// We add (len(kvList) % 2) here to handle the case where there is an
// odd number of elements in a kvList.
// If there is an odd number, then the last element in the slice will
// have the value 'null'.
for i2 := len(kvList) - 2 + (len(kvList) % 2); i2 >= 0; i2 -= 2 {
k := kvList[i2]
// if we have already seen this key, do not include it again
if _, ok := seenKeys[k]; ok {
continue
}
// make a note that we've observed a new key
seenKeys[k] = struct{}{}
// attempt to obtain the value of the key
var v interface{}
// i2+1 should only ever be out of bounds if we handling the first
// iteration over a slice with an odd number of elements
if i2+1 < len(kvList) {
v = kvList[i2+1]
}
// add this KV tuple to the *start* of the output list to maintain
// the original order as we are iterating over the slice backwards
outs[i] = append([]interface{}{k, v}, outs[i]...)
}
}
return outs
}

// MergeKVs deduplicates elements provided in two key/value slices.
//
// Keys in each slice are expected to be unique, so duplicates can only occur
Expand Down
106 changes: 33 additions & 73 deletions internal/serialize/keyvalues_test.go
View file
Expand Up @@ -153,106 +153,66 @@ No whitespace.`,

func TestDuplicates(t *testing.T) {
for name, test := range map[string]struct {
first, second []interface{}
expectedTrimmed [][]interface{}
expectedMerged []interface{}
first, second []interface{}
expected []interface{}
}{
"empty": {
expectedTrimmed: [][]interface{}{{}, {}},
},
"empty": {},
"no-duplicates": {
first: makeKV("a", 3),
second: makeKV("b", 3),
expectedTrimmed: [][]interface{}{
makeKV("a", 3),
makeKV("b", 3),
},
expectedMerged: append(makeKV("a", 3), makeKV("b", 3)...),
first: makeKV("a", 3),
second: makeKV("b", 3),
expected: append(makeKV("a", 3), makeKV("b", 3)...),
},
"all-duplicates": {
first: makeKV("a", 3),
second: makeKV("a", 3),
expectedTrimmed: [][]interface{}{
{},
makeKV("a", 3),
},
expectedMerged: makeKV("a", 3),
first: makeKV("a", 3),
second: makeKV("a", 3),
expected: makeKV("a", 3),
},
"start-duplicate": {
first: append([]interface{}{"x", 1}, makeKV("a", 3)...),
second: append([]interface{}{"x", 2}, makeKV("b", 3)...),
expectedTrimmed: [][]interface{}{
makeKV("a", 3),
append([]interface{}{"x", 2}, makeKV("b", 3)...),
},
expectedMerged: append(append(makeKV("a", 3), "x", 2), makeKV("b", 3)...),
first: append([]interface{}{"x", 1}, makeKV("a", 3)...),
second: append([]interface{}{"x", 2}, makeKV("b", 3)...),
expected: append(append(makeKV("a", 3), "x", 2), makeKV("b", 3)...),
},
"subset-first": {
first: append([]interface{}{"x", 1}, makeKV("a", 3)...),
second: append([]interface{}{"x", 2}, makeKV("a", 3)...),
expectedTrimmed: [][]interface{}{
{},
append([]interface{}{"x", 2}, makeKV("a", 3)...),
},
expectedMerged: append([]interface{}{"x", 2}, makeKV("a", 3)...),
first: append([]interface{}{"x", 1}, makeKV("a", 3)...),
second: append([]interface{}{"x", 2}, makeKV("a", 3)...),
expected: append([]interface{}{"x", 2}, makeKV("a", 3)...),
},
"subset-second": {
first: append([]interface{}{"x", 1}, makeKV("a", 1)...),
second: append([]interface{}{"x", 2}, makeKV("b", 2)...),
expectedTrimmed: [][]interface{}{
makeKV("a", 1),
append([]interface{}{"x", 2}, makeKV("b", 2)...),
},
expectedMerged: append(append(makeKV("a", 1), "x", 2), makeKV("b", 2)...),
first: append([]interface{}{"x", 1}, makeKV("a", 1)...),
second: append([]interface{}{"x", 2}, makeKV("b", 2)...),
expected: append(append(makeKV("a", 1), "x", 2), makeKV("b", 2)...),
},
"end-duplicate": {
first: append(makeKV("a", 3), "x", 1),
second: append(makeKV("b", 3), "x", 2),
expectedTrimmed: [][]interface{}{
makeKV("a", 3),
append(makeKV("b", 3), "x", 2),
},
expectedMerged: append(makeKV("a", 3), append(makeKV("b", 3), "x", 2)...),
first: append(makeKV("a", 3), "x", 1),
second: append(makeKV("b", 3), "x", 2),
expected: append(makeKV("a", 3), append(makeKV("b", 3), "x", 2)...),
},
"middle-duplicate": {
first: []interface{}{"a-0", 0, "x", 1, "a-1", 2},
second: []interface{}{"b-0", 0, "x", 2, "b-1", 2},
expectedTrimmed: [][]interface{}{
{"a-0", 0, "a-1", 2},
{"b-0", 0, "x", 2, "b-1", 2},
},
expectedMerged: []interface{}{"a-0", 0, "a-1", 2, "b-0", 0, "x", 2, "b-1", 2},
first: []interface{}{"a-0", 0, "x", 1, "a-1", 2},
second: []interface{}{"b-0", 0, "x", 2, "b-1", 2},
expected: []interface{}{"a-0", 0, "a-1", 2, "b-0", 0, "x", 2, "b-1", 2},
},
"internal-duplicates": {
first: []interface{}{"a", 0, "x", 1, "a", 2},
second: []interface{}{"b", 0, "x", 2, "b", 2},
expectedTrimmed: [][]interface{}{
{"a", 2},
{"x", 2, "b", 2},
},
// This is the case where Merged keeps key/value pairs
// that were already duplicated inside the slices, for
// performance.
expectedMerged: []interface{}{"a", 0, "a", 2, "b", 0, "x", 2, "b", 2},
expected: []interface{}{"a", 0, "a", 2, "b", 0, "x", 2, "b", 2},
},
} {
t.Run(name, func(t *testing.T) {
t.Run("TrimDuplicates", func(t *testing.T) {
actual := serialize.TrimDuplicates(test.first, test.second)
expectEqual(t, "trimmed key/value pairs", test.expectedTrimmed, actual)
})

t.Run("Merged", func(t *testing.T) {
actual := serialize.MergeKVs(test.first, test.second)
expectEqual(t, "merged key/value pairs", test.expectedMerged, actual)
expectEqual(t, "merged key/value pairs", test.expected, actual)
})
})
}
}

// BenchmarkTrimDuplicates checks performance when TrimDuplicates is called with two slices.
// BenchmarkMergeKVs checks performance when MergeKVs is called with two slices.
// In practice that is how the function is used.
func BenchmarkTrimDuplicates(b *testing.B) {
func BenchmarkMergeKVs(b *testing.B) {
for firstLength := 0; firstLength < 10; firstLength++ {
firstA := makeKV("a", firstLength)
for secondLength := 0; secondLength < 10; secondLength++ {
Expand All @@ -262,7 +222,7 @@ func BenchmarkTrimDuplicates(b *testing.B) {
// This is the most common case: all key/value pairs are kept.
b.Run("no-duplicates", func(b *testing.B) {
expected := append(firstA, secondB...)
benchTrimDuplicates(b, expected, firstA, secondB)
benchMergeKVs(b, expected, firstA, secondB)
})

// Fairly unlikely...
Expand All @@ -272,7 +232,7 @@ func BenchmarkTrimDuplicates(b *testing.B) {
expected = firstA[secondLength*2:]
}
expected = append(expected, secondA...)
benchTrimDuplicates(b, expected, firstA, secondA)
benchMergeKVs(b, expected, firstA, secondA)
})

// First entry is the same.
Expand All @@ -282,7 +242,7 @@ func BenchmarkTrimDuplicates(b *testing.B) {
second := []interface{}{"x", 1}
second = append(second, secondB...)
expected := append(firstA, second...)
benchTrimDuplicates(b, expected, first, second)
benchMergeKVs(b, expected, first, second)
})

// Last entry is the same.
Expand All @@ -292,7 +252,7 @@ func BenchmarkTrimDuplicates(b *testing.B) {
second := secondB[:]
second = append(second, "x", 1)
expected := append(firstA, second...)
benchTrimDuplicates(b, expected, first, second)
benchMergeKVs(b, expected, first, second)
})
})
}
Expand All @@ -310,7 +270,7 @@ func makeKV(prefix string, length int) []interface{} {
return kv
}

func benchTrimDuplicates(b *testing.B, expected []interface{}, first, second []interface{}) {
func benchMergeKVs(b *testing.B, expected []interface{}, first, second []interface{}) {
if len(expected) == 0 {
expected = nil
}
Expand Down

0 comments on commit eb21886

Please sign in to comment.