sortutil icon indicating copy to clipboard operation
sortutil copied to clipboard

Sorting by Fields Doesn't Always Work

Open danrzeppa opened this issue 10 years ago • 2 comments

When you have a slice of pointers to structs and you sort by a field within the struct, it doesn't always work. If you add this test case to the all_test.go file and run the tests it fails.

func TestAscByFieldInt64_Pointers(t *testing.T) {
    now := time.Now()
    is := []*Item{
        {5, "", now, false},
        {2, "", now, false},
        {1, "", now, false},
        {4, "", now, false},
        {3, "", now, false},
    }
    AscByField(is, "Id")
    for i, v := range is {
        if v.Id != int64(i+1) {
            t.Errorf("is[%d].Id is not %d, but %d", i, i+1, v.Id)
        }
    }
}

The sequence of test numbers matters. Some initial ordering of values will still work, while some will not.

I believe the issue is related to the Sorter.vals slice getting out of sync with the Sorter.Slice when swapping elements. When a swap happens on the Sorter.Slice, the pointers to the structs are properly moved, but because the Sorter.vals slice contains reflect.Value "pointing" to a field, indexing the Sorter.vals array will now give you the wrong value for the element in the slice.

The test case above will pass by adding

s.vals[i], s.vals[j] = s.vals[j], s.vals[i]

to the bottom of the Sorter.Swap func. But an array of values (not pointers) will now fail.

I tried modifying the code so that the Getter would return a boolean indicating if the values needed to swapped (based on if indirection was needed), but I couldn't get all the Benchmarks to pass, and I ran out of time to continue digging into it.

I can share what I did get done, although I'm not 100% sure it is the best way to go about fixing the issue.

danrzeppa avatar Jun 21 '15 01:06 danrzeppa

Ouch, interesting. I'll take a look.

patrickmn avatar Jun 22 '15 20:06 patrickmn

we also ran into this same issue trying to sort a slice of struct pointers. Would be nice to have a fix

jdonboch avatar Jan 13 '16 22:01 jdonboch