Changing the sorting for terms aggs,

[thanodnl]

This pull request fixes the sorting of aggregations as discussed in issue #5236.

It forces the NaN values to be pushed to the bottom of the list, no matter if you are sorting asc/desc. This way top lists show the most relevant aggregates in the top. Also it actually does what the comments above it already said it should do.

Closes #5236.

[jpountz]

Maybe this should even be

if (Double.isNaN(v1)) {
    if (Double.isNaN(v2)) {
        return 0;
    } else {
        return 1;
    }
}

[uboness]

if (Double.isNaN(v1)) {
    return Double.isNaN(v2) ? 0 : 1;
}

[thanodnl]

I think the suggestion of @uboness is more readable than the extra if-statement and indents. I'll change it to that one for a complete implementation of the comparator contract.

Thnx

[jpountz]

The fix look good. Can you write a unit test for this? (If that looks complicated to you, I can take care of this, just let me know!)

[thanodnl]

I've been looking into the test, but I find it hard to see where to add what :).

I found StringTermsTests (and my changes didn't break it) where some tests for terms aggregations are, including sorting on sub aggregate. But for this test I need to change the source dataset in a way I cannot really comprehense.

Would you mind adding a test?

[jpountz]

Indeed, this would be easier to have a different test case.

Would you mind adding a test?

Sure, I will work on it!

[uboness]

left one comment... other than that LGTM