diff --git a/docs/changelog/94734.yaml b/docs/changelog/94734.yaml new file mode 100644 index 0000000000000..aea8fe534a37f --- /dev/null +++ b/docs/changelog/94734.yaml @@ -0,0 +1,6 @@ +pr: 94734 +summary: Fix bug where `geo_line` does not respect `sort_order` +area: Geo +type: bug +issues: + - 94733 diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLine.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLine.java index 89cbeef0a55f1..cf8605c0cf382 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLine.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLine.java @@ -110,10 +110,6 @@ public InternalAggregation reduce(List aggregations, Aggreg MergedGeoLines mergedGeoLines = new MergedGeoLines(internalGeoLines, finalSize, sortOrder); mergedGeoLines.merge(); - // 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(); - } return new InternalGeoLine( name, mergedGeoLines.getFinalPoints(), diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/GeoLineAggregatorTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/GeoLineAggregatorTests.java index abc897b25a08e..31db0c26c6fdf 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/GeoLineAggregatorTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/GeoLineAggregatorTests.java @@ -323,13 +323,13 @@ private void testAggregator(SortOrder sortOrder) throws IOException { int encodedLon = GeoEncodingUtils.encodeLongitude(point.getLon()); long lonLat = (((long) encodedLon) << 32) | encodedLat & 0xffffffffL; points[i] = lonLat; - sortValues[i] = SortOrder.ASC.equals(sortOrder) ? i : numPoints - i; + sortValues[i] = i; } int lineSize = Math.min(numPoints, size); // re-sort line to be ascending long[] linePoints = Arrays.copyOf(points, lineSize); double[] lineSorts = Arrays.copyOf(sortValues, lineSize); - new PathArraySorter(linePoints, lineSorts, SortOrder.ASC).sort(); + new PathArraySorter(linePoints, lineSorts, sortOrder).sort(); lines.put(String.valueOf(groupOrd), new InternalGeoLine("_name", linePoints, lineSorts, null, complete, true, sortOrder, size)); diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLineTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLineTests.java index 13bc5d5fb8c41..78b9e7f679b4f 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLineTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLineTests.java @@ -134,10 +134,6 @@ protected void assertReduced(InternalGeoLine reduced, List inpu long[] finalCappedPoints = Arrays.copyOf(finalList, Math.min(reduced.size(), mergedLength)); double[] finalCappedSortVals = Arrays.copyOf(finalSortVals, Math.min(reduced.size(), mergedLength)); - if (SortOrder.DESC.equals(reduced.sortOrder())) { - new PathArraySorter(finalCappedPoints, finalCappedSortVals, SortOrder.ASC).sort(); - } - assertArrayEquals(finalCappedSortVals, reduced.sortVals(), 0d); assertArrayEquals(finalCappedPoints, reduced.line()); } diff --git a/x-pack/plugin/spatial/src/test/resources/rest-api-spec/test/50_geoline.yml b/x-pack/plugin/spatial/src/test/resources/rest-api-spec/test/50_geoline.yml deleted file mode 100644 index ca4a0873c33c4..0000000000000 --- a/x-pack/plugin/spatial/src/test/resources/rest-api-spec/test/50_geoline.yml +++ /dev/null @@ -1,59 +0,0 @@ ---- -"Test geoline agg": - - do: - indices.create: - index: locations - body: - mappings: - properties: - location: - type: geo_point - rank: - type: double - - - do: - bulk: - refresh: true - body: - - index: - _index: locations - _id: "1" - - '{"location": [13.37139831, 47.82930284], "rank": 2.0 }' - - index: - _index: locations - _id: "2" - - '{"location": [13.3784208402, 47.88832084022], "rank": 0.0 }' - - index: - _index: locations - _id: "3" - - '{"location": [13.371830148701, 48.2084200148], "rank": 1.2 }' - - - do: - search: - rest_total_hits_as_int: true - index: locations - size: 0 - body: - aggs: - path: - geo_line: - include_sort: true - geo_point: - field: location - sort: - field: rank - - match: { hits.total: 3 } - - match: { aggregations.path.type: "Feature" } - - match: { aggregations.path.geometry.type: "LineString" } - - length: { aggregations.path.geometry.coordinates: 3 } - - match: { aggregations.path.geometry.coordinates.0.0: 13.378421 } - - match: { aggregations.path.geometry.coordinates.0.1: 47.888321 } - - match: { aggregations.path.geometry.coordinates.1.0: 13.37183 } - - match: { aggregations.path.geometry.coordinates.1.1: 48.20842 } - - match: { aggregations.path.geometry.coordinates.2.0: 13.371398 } - - match: { aggregations.path.geometry.coordinates.2.1: 47.829303 } - - is_true: aggregations.path.properties.complete - - length: { aggregations.path.properties.sort_values: 3 } - - match: { aggregations.path.properties.sort_values.0: 0.0 } - - match: { aggregations.path.properties.sort_values.1: 1.2 } - - match: { aggregations.path.properties.sort_values.2: 2.0 } diff --git a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/spatial/60_geo_line.yml b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/spatial/60_geo_line.yml index c75e687ec78f1..523fc1a0a7bd7 100644 --- a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/spatial/60_geo_line.yml +++ b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/spatial/60_geo_line.yml @@ -11,6 +11,17 @@ setup: position: type: geo_point + - do: + indices.create: + index: locations + body: + mappings: + properties: + location: + type: geo_point + rank: + type: double + - do: indices.create: index: test1 @@ -49,6 +60,18 @@ setup: {"index":{}} {"position": "POINT(4.914722 52.371667)", "race_id": "Amsterdam", "timestamp": 10} + - do: + bulk: + index: locations + refresh: true + body: | + {"index":{}} + {"location": [13.37139831, 47.82930284], "rank": 2.0 } + {"index":{}} + {"location": [13.3784208402, 47.88832084022], "rank": 0.0 } + {"index":{}} + {"location": [13.371830148701, 48.2084200148], "rank": 1.2 } + - do: bulk: index: test1 @@ -79,6 +102,128 @@ setup: - do: indices.refresh: { } +--- +"Test geo_line aggregation on geo points sort by double and include_sort": + - do: + search: + rest_total_hits_as_int: true + index: locations + size: 0 + body: + aggs: + path: + geo_line: + include_sort: true + point: + field: location + sort: + field: rank + - match: { hits.total: 3 } + - match: { aggregations.path.type: "Feature" } + - match: { aggregations.path.geometry.type: "LineString" } + - length: { aggregations.path.geometry.coordinates: 3 } + - match: { aggregations.path.geometry.coordinates.0.0: 13.378421 } + - match: { aggregations.path.geometry.coordinates.0.1: 47.888321 } + - match: { aggregations.path.geometry.coordinates.1.0: 13.37183 } + - match: { aggregations.path.geometry.coordinates.1.1: 48.20842 } + - match: { aggregations.path.geometry.coordinates.2.0: 13.371398 } + - match: { aggregations.path.geometry.coordinates.2.1: 47.829303 } + - is_true: aggregations.path.properties.complete + - length: { aggregations.path.properties.sort_values: 3 } + - match: { aggregations.path.properties.sort_values.0: 0.0 } + - match: { aggregations.path.properties.sort_values.1: 1.2 } + - match: { aggregations.path.properties.sort_values.2: 2.0 } + +--- +"Test geo_line aggregation on geo points reverse sort by double and include_sort": + - do: + search: + rest_total_hits_as_int: true + index: locations + size: 0 + body: + aggs: + path: + geo_line: + include_sort: true + sort_order: "desc" + point: + field: location + sort: + field: rank + - match: { hits.total: 3 } + - match: { aggregations.path.type: "Feature" } + - match: { aggregations.path.geometry.type: "LineString" } + - length: { aggregations.path.geometry.coordinates: 3 } + - match: { aggregations.path.geometry.coordinates.0.0: 13.371398 } + - match: { aggregations.path.geometry.coordinates.0.1: 47.829303 } + - match: { aggregations.path.geometry.coordinates.1.0: 13.37183 } + - match: { aggregations.path.geometry.coordinates.1.1: 48.20842 } + - match: { aggregations.path.geometry.coordinates.2.0: 13.378421 } + - match: { aggregations.path.geometry.coordinates.2.1: 47.888321 } + - is_true: aggregations.path.properties.complete + - length: { aggregations.path.properties.sort_values: 3 } + - match: { aggregations.path.properties.sort_values.0: 2.0 } + - match: { aggregations.path.properties.sort_values.1: 1.2 } + - match: { aggregations.path.properties.sort_values.2: 0.0 } + +--- +"Test geo_line aggregation on geo points limit size": + - do: + search: + rest_total_hits_as_int: true + index: locations + size: 0 + body: + aggs: + path: + geo_line: + size: 2 + point: + field: location + sort: + field: rank + - match: { hits.total: 3 } + - match: { aggregations.path.type: "Feature" } + - match: { aggregations.path.geometry.type: "LineString" } + - length: { aggregations.path.geometry.coordinates: 2 } + - match: { aggregations.path.geometry.coordinates.0.0: 13.378421 } + - match: { aggregations.path.geometry.coordinates.0.1: 47.888321 } + - match: { aggregations.path.geometry.coordinates.1.0: 13.37183 } + - match: { aggregations.path.geometry.coordinates.1.1: 48.20842 } + - is_false: aggregations.path.properties.complete + +--- +"Test geo_line aggregation on geo points limit and sort": + - do: + search: + rest_total_hits_as_int: true + index: locations + size: 0 + body: + aggs: + path: + geo_line: + size: 2 + include_sort: true + sort_order: "desc" + point: + field: location + sort: + field: rank + - match: { hits.total: 3 } + - match: { aggregations.path.type: "Feature" } + - match: { aggregations.path.geometry.type: "LineString" } + - length: { aggregations.path.geometry.coordinates: 2 } + - match: { aggregations.path.geometry.coordinates.0.0: 13.371398 } + - match: { aggregations.path.geometry.coordinates.0.1: 47.829303 } + - match: { aggregations.path.geometry.coordinates.1.0: 13.37183 } + - match: { aggregations.path.geometry.coordinates.1.1: 48.20842 } + - is_false: aggregations.path.properties.complete + - length: { aggregations.path.properties.sort_values: 2 } + - match: { aggregations.path.properties.sort_values.0: 2.0 } + - match: { aggregations.path.properties.sort_values.1: 1.2 } + --- "Test geo_line aggregation on geo points with no grouping": - do: