Fix bug 94733 geo_line not respecting sort_order

[craigtaverner]

Fixes bug where geo_line did not support sort_order, silently ignoring the DESC option.

The documentation for geo_line claims that the option sort_order can be either ASC (default) or DESC:

This option accepts one of two values: "ASC", "DESC".
The line is sorted in ascending order by the sort key when set to "ASC", and in descending with "DESC".

However, when testing this, no matter which you choose the actual order of points in the result is always ASC by the value of the sort field.

Fixes #94733

[elasticsearchmachine]

Pinging @elastic/es-analytics-geo (Team:Analytics)

[elasticsearchmachine]

Hi @craigtaverner, I've created a changelog YAML for you.

[craigtaverner]

@elasticmachine run elasticsearch-ci/docs

[craigtaverner]

The original PR that created geo_line aggregation and added the explicit sort, has a feature description claiming that sort_order: DESC is supported (gives this as the first example). However, that PR was created in April 2019, and only merged in Nov 2020 with 34 commits, so there is a lot of hidden history. It is hard to see what the reasoning was for deliberately killing this behaviour. The code that forces ASC on the sort says:

        // the final reduce should always be in ascending order
        if (reduceContext.isFinalReduce() && SortOrder.DESC.equals(sortOrder)) {
            new PathArraySorter(mergedGeoLines.getFinalPoints(), mergedGeoLines.getFinalSortValues(), SortOrder.ASC).sort();
        }

See #41612

[craigtaverner]

I've found the commit that added the final sort. It has the commit message:

add size param

baaea0e#diff-cc94b6c97b605e29bde7bb31a0d4af9b1a368818e7261c48a34590c93195ee8e

My suspicion is that when implementing the size parameter he either found it did not work correctly without re-sorting, or he suspected it would not work correctly. Either way there are no tests that fail if we do not sort like this, so either the necessary tests were never written, or this sort is genuinely not required. I suspect the latter.

[craigtaverner]

The kibana team feel that this was not done for Kibana reasons. The most likely remaining reason was that it was something done during the work on add size param. My suspicion was that it was an unnecessary change based on a misunderstanding. Quite possibly a temporary situation during development. In any case as long as we have tests that assert correct behaviour of the combination of size and sort_order we should be good. I will verify and add any missing tests.

[iverase]

LGTM