Skip to content

Sorting by Fields Doesn't Always Work #2

@danrzeppa

Description

@danrzeppa

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions