Refactor equals

[aledbf]

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #3956

Special notes for your reviewer:

[aledbf]

/hold

[aledbf]

[]string{"2", "1", "3"}, []string{"1", "2", "3"}, false, Should be true. That is one of the reasons why we cannot use reflect.DeepEqual. Also, we cannot assume the lists are ordered.

…

On Wed, Apr 3, 2019 at 2:46 AM dongqi ***@***.***> wrote: why don't we use the equal function like this func StringSliceEqual(a, b []string) bool { if len(a) != len(b) { return false } if (a == nil) != (b == nil) { return false } for i, v := range a { if v != b[i] { return false } } return true } here is the benchmark test equal.go package equal import "reflect" func StringSliceReflectEqual(a, b []string) bool { return reflect.DeepEqual(a, b) } func StringSliceEqual(a, b []string) bool { if len(a) != len(b) { return false } if (a == nil) != (b == nil) { return false } for i, v := range a { if v != b[i] { return false } } return true } func StringElementsMatch(listA, listB []string) bool { if len(listA) != len(listB) { return false } visited := make([]bool, len(listB)) for i := 0; i < len(listA); i++ { element := listA[i] found := false for j := 0; j < len(listB); j++ { if visited[j] { continue } if listB[j] == element { visited[j] = true found = true break } } if !found { return false } } return true } equal_test.go package equal import "testing" var stringSliceEqualTests = []struct { a []string b []string out bool }{ { []string{"1", "2", "3"}, []string{"1", "2", "3"}, true, }, { []string{"2", "1", "3"}, []string{"1", "2", "3"}, false, }, { make([]string, 0, 3), make([]string, 0, 3), true, }, { make([]string, 0, 3), make([]string, 0, 5), true, }, { []string{"1", "1", "1"}[0:1], []string{"1", "1", "1"}[1:2], true, }, { make([]string, 0, 3), nil, false, }, } func TestStringSliceReflectEqual(t *testing.T) { for _, test := range stringSliceEqualTests { actual := StringSliceReflectEqual(test.a, test.b) if actual != test.out { t.Errorf("ReflectEqual(%#v, %#v) = %v; want %v", test.a, test.b, actual, test.out) } } } func TestStringSliceEqual(t *testing.T) { for _, test := range stringSliceEqualTests { actual := StringSliceEqual(test.a, test.b) if actual != test.out { t.Errorf("Equal(%#v, %#v) = %v; want %v", test.a, test.b, actual, test.out) } } } func TestStringElementsMatch(t *testing.T) { for _, test := range stringSliceEqualTests { actual := StringElementsMatch(test.a, test.b) if actual != test.out { t.Errorf("Equal(%#v, %#v) = %v; want %v", test.a, test.b, actual, test.out) } } } func BenchmarkStringSliceReflectEqual(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { for _, test := range stringSliceEqualTests { StringSliceReflectEqual(test.a, test.b) } } } func BenchmarkStringSliceEqual(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { for _, test := range stringSliceEqualTests { StringSliceEqual(test.a, test.b) } } } func BenchmarkStringElementsMatch(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { for _, test := range stringSliceEqualTests { StringElementsMatch(test.a, test.b) } } } run go test -bench=. -run=none, we can get the result below: goos: linux goarch: amd64 pkg: Learning/equal BenchmarkStringSliceReflectEqual-4 1000000 1073 ns/op BenchmarkStringSliceEqual-4 30000000 43.3 ns/op BenchmarkStringElementsMatch-4 20000000 97.0 ns/op PASS ok Learning/equal 4.473s — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3958 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJ3Iwz2Yn3NtdQKiezsn-RvhKB_qcQcks5vdEBPgaJpZM4cYHhh> .

[ElvinEfendi]

/s/result/sameResult

[ElvinEfendi]

can we not make this general enough so that we don't repeat the same logic for different types? maybe something like following in sets package?

func Compare(s1, s2 []interface, equalFunc func(e1 interface, e2 interface) bool) bool {

with this you can still have type specific helper functions, but based on this common function. that way we would not be repeating the same logic in multiple functions.

[aledbf]

can we not make this general enough so that we don't repeat the same logic for different types? maybe something like following in sets package?

I started doing just that but it implies using reflection

func Compare(listA, listB interface{}, equalFunc func(e1, e2 interface{}) bool) bool {
	a := reflect.ValueOf(listA)
	log.Println(reflect.TypeOf(a).Kind())
	if reflect.TypeOf(a).Kind() != reflect.Slice &&
		reflect.TypeOf(a).Kind() != reflect.Struct &&
		reflect.TypeOf(a).Kind() != reflect.Array {
		return false
	}

	b := reflect.ValueOf(listB)
	if reflect.TypeOf(b).Kind() != reflect.Slice &&
		reflect.TypeOf(b).Kind() != reflect.Array &&
		reflect.TypeOf(b).Kind() != reflect.Struct {
		return false
	}

	if a.Len() != b.Len() {
		return false
	}

	visited := make([]bool, b.Len())

	for i := 0; i < a.Len(); i++ {
		element := a.Index(i).Interface()

		found := false
		for j := 0; j < b.Len(); j++ {
			if visited[j] {
				continue
			}

			if equalFunc(element, b.Index(j).Interface()) {
				visited[j] = true
				found = true
				break
			}
		}

		if !found {
			return false
		}
	}

	return true
}

If you are ok with that I can change the code

Edit: not sure if some of the equalFunc would require type casting or not

[ElvinEfendi]

/lgtm

[aledbf]

@ElvinEfendi this is the working implementation https://gist.github.com/aledbf/5ee02ca3959cb640ae83760145aef4ca

[ElvinEfendi]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ElvinEfendi,aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[teresadq]

@aledbf why the type struct is iterative?

[aledbf]

@dongqi1990 fixed by #3980