Clever / optimus

Concurrently extract, transform, and load tables of data in Go
Apache License 2.0
34 stars 7 forks source link

add transforms.StableSort #27

Closed azylman closed 9 years ago

azylman commented 9 years ago

For those times when you want a stable sort.

I didn't see a need to test that the result is stably sorted - that seemed to me redundant with existing tests for sort.Stable. I mostly want to test that we correctly use + interpret the Less function that's passed in, so I re-used the tests for unstable Sort.

d-sparks commented 9 years ago

There should be a test that differentiates the Stable sort from the unstable one.

azylman commented 9 years ago

@d-sparks did you see my comment about testing the "stableness"? Basically, the output order from Sort isn't guaranteed so any test from that is guaranteed to be flaky - sometimes, Sort might just output in the sorted order. Additionally, at that point you're just testing the standard library.

d-sparks commented 9 years ago

I'll buy that you can't negatively test the unstable sort if the order is random (unless you made a probabilistic test that sorts many times, which isn't worth it). But I would like to push back on the fact that we're testing the standard library. If somebody came along and re-implemented our sorts, and we were relying on the tests in the standard library, we would suddenly have no tests.

I highly favor a case where there are a few rows that have the same sort value, and ensuring they don't get mixed up.

azylman commented 9 years ago

@d-sparks added a test for "stableness" + rebased

d-sparks commented 9 years ago

lgtm!