Sorts column data using fastutil .sort methods #778
Conversation
| @@ -30,6 +29,14 @@ public void unique() { | |||
| assertArrayEquals(arr, new double[] {1.0, 2.0, 3.0, 4.0, 5.0}); | |||
| } | |||
|
|
|||
| @Test | |||
| public void createThenSortAscending() { | |||
There was a problem hiding this comment.
How is this different than the existing sortAscending test?
There was a problem hiding this comment.
When you create an empty column like DoubleColumn.create("test") it actually creates a DEFAULT_ARRAY_SIZE array which has zeros. It showcases the bug nicely.
| @@ -297,12 +296,12 @@ public BooleanColumn copy() { | |||
|
|
|||
| @Override | |||
| public void sortAscending() { | |||
| ByteArrays.mergeSort(data.elements()); | |||
| data.sort(descendingByteComparator.reversed() /* or ByteComparators.NATURAL_COMPARATOR */); | |||
There was a problem hiding this comment.
This is kind of a double negative. I'd prefer the ByteComparators.NATURAL_COMPARATOR option rather than reversing a reverse comparator
There was a problem hiding this comment.
Yeah, I'd use the natural comparators all over the place. But incrementally it made sense in a way to use whatever comparator you had there (just in case there was some specific logic -- I didn't really check them all).
I will update the PR to use those comparators.
| } | ||
|
|
||
| @Override | ||
| public void sortDescending() { | ||
| ByteArrays.mergeSort(data.elements(), descendingByteComparator); | ||
| data.sort(descendingByteComparator); |
There was a problem hiding this comment.
Perhaps we should remove descendingByteComparator and use ByteComparators.OPPOSITE_COMPARATOR here for consistency if we change sortAscending
|
See the updated PR. |
|
Thanks for the fix! |
Thanks for contributing.
Description
Sorts the fastutil collections using the proper
sortmethod and avoids accessing the backing array via.elements()since it will generally have invalid data or zeros in there.Closes issue #775
Testing
Two tests, and everything existing still runs which is promising.