Skip to content

Commit e5f03ad

Browse files
craigtavernercraigtaverner
authored
Fix bug 94733 geo_line not respecting sort_order (#94734)
* Move unused geoline tests into correct location The original geo_line PR included two yaml test files, one of which was never used. This commit moves that test into the correct place, and fixes a syntax error in the test. * Fix geo_line sort order (#94733) The code was re-sorting to ASC explicitly in the reduce phase. No explanation was given. Removing this extra sort causes the new yaml test to pass. Two failing java tests were fixed by no longer explicitly expected ASC data when order was DESC. * Update docs/changelog/94734.yaml * Improve changelog text * Added test for size in geo_line * Add geo_line sort and limit test
1 parent 5e327e2 commit e5f03ad

File tree

6 files changed

+153
-69
lines changed

6 files changed

+153
-69
lines changed

‎docs/changelog/94734.yaml

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 94734
2+
summary: Fix bug where `geo_line` does not respect `sort_order`
3+
area: Geo
4+
type: bug
5+
issues:
6+
- 94733

‎x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLine.java

-4
Original file line numberDiff line numberDiff line change
@@ -110,10 +110,6 @@ public InternalAggregation reduce(List<InternalAggregation> aggregations, Aggreg
110110

111111
MergedGeoLines mergedGeoLines = new MergedGeoLines(internalGeoLines, finalSize, sortOrder);
112112
mergedGeoLines.merge();
113-
// the final reduce should always be in ascending order
114-
if (reduceContext.isFinalReduce() && SortOrder.DESC.equals(sortOrder)) {
115-
new PathArraySorter(mergedGeoLines.getFinalPoints(), mergedGeoLines.getFinalSortValues(), SortOrder.ASC).sort();
116-
}
117113
return new InternalGeoLine(
118114
name,
119115
mergedGeoLines.getFinalPoints(),

‎x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/GeoLineAggregatorTests.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -323,13 +323,13 @@ private void testAggregator(SortOrder sortOrder) throws IOException {
323323
int encodedLon = GeoEncodingUtils.encodeLongitude(point.getLon());
324324
long lonLat = (((long) encodedLon) << 32) | encodedLat & 0xffffffffL;
325325
points[i] = lonLat;
326-
sortValues[i] = SortOrder.ASC.equals(sortOrder) ? i : numPoints - i;
326+
sortValues[i] = i;
327327
}
328328
int lineSize = Math.min(numPoints, size);
329329
// re-sort line to be ascending
330330
long[] linePoints = Arrays.copyOf(points, lineSize);
331331
double[] lineSorts = Arrays.copyOf(sortValues, lineSize);
332-
new PathArraySorter(linePoints, lineSorts, SortOrder.ASC).sort();
332+
new PathArraySorter(linePoints, lineSorts, sortOrder).sort();
333333

334334
lines.put(String.valueOf(groupOrd), new InternalGeoLine("_name", linePoints, lineSorts, null, complete, true, sortOrder, size));
335335

‎x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLineTests.java

-4
Original file line numberDiff line numberDiff line change
@@ -134,10 +134,6 @@ protected void assertReduced(InternalGeoLine reduced, List<InternalGeoLine> inpu
134134
long[] finalCappedPoints = Arrays.copyOf(finalList, Math.min(reduced.size(), mergedLength));
135135
double[] finalCappedSortVals = Arrays.copyOf(finalSortVals, Math.min(reduced.size(), mergedLength));
136136

137-
if (SortOrder.DESC.equals(reduced.sortOrder())) {
138-
new PathArraySorter(finalCappedPoints, finalCappedSortVals, SortOrder.ASC).sort();
139-
}
140-
141137
assertArrayEquals(finalCappedSortVals, reduced.sortVals(), 0d);
142138
assertArrayEquals(finalCappedPoints, reduced.line());
143139
}

‎x-pack/plugin/spatial/src/test/resources/rest-api-spec/test/50_geoline.yml

-59
This file was deleted.

‎x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/spatial/60_geo_line.yml

+145
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,17 @@ setup:
1111
position:
1212
type: geo_point
1313

14+
- do:
15+
indices.create:
16+
index: locations
17+
body:
18+
mappings:
19+
properties:
20+
location:
21+
type: geo_point
22+
rank:
23+
type: double
24+
1425
- do:
1526
indices.create:
1627
index: test1
@@ -49,6 +60,18 @@ setup:
4960
{"index":{}}
5061
{"position": "POINT(4.914722 52.371667)", "race_id": "Amsterdam", "timestamp": 10}
5162
63+
- do:
64+
bulk:
65+
index: locations
66+
refresh: true
67+
body: |
68+
{"index":{}}
69+
{"location": [13.37139831, 47.82930284], "rank": 2.0 }
70+
{"index":{}}
71+
{"location": [13.3784208402, 47.88832084022], "rank": 0.0 }
72+
{"index":{}}
73+
{"location": [13.371830148701, 48.2084200148], "rank": 1.2 }
74+
5275
- do:
5376
bulk:
5477
index: test1
@@ -79,6 +102,128 @@ setup:
79102
- do:
80103
indices.refresh: { }
81104

105+
---
106+
"Test geo_line aggregation on geo points sort by double and include_sort":
107+
- do:
108+
search:
109+
rest_total_hits_as_int: true
110+
index: locations
111+
size: 0
112+
body:
113+
aggs:
114+
path:
115+
geo_line:
116+
include_sort: true
117+
point:
118+
field: location
119+
sort:
120+
field: rank
121+
- match: { hits.total: 3 }
122+
- match: { aggregations.path.type: "Feature" }
123+
- match: { aggregations.path.geometry.type: "LineString" }
124+
- length: { aggregations.path.geometry.coordinates: 3 }
125+
- match: { aggregations.path.geometry.coordinates.0.0: 13.378421 }
126+
- match: { aggregations.path.geometry.coordinates.0.1: 47.888321 }
127+
- match: { aggregations.path.geometry.coordinates.1.0: 13.37183 }
128+
- match: { aggregations.path.geometry.coordinates.1.1: 48.20842 }
129+
- match: { aggregations.path.geometry.coordinates.2.0: 13.371398 }
130+
- match: { aggregations.path.geometry.coordinates.2.1: 47.829303 }
131+
- is_true: aggregations.path.properties.complete
132+
- length: { aggregations.path.properties.sort_values: 3 }
133+
- match: { aggregations.path.properties.sort_values.0: 0.0 }
134+
- match: { aggregations.path.properties.sort_values.1: 1.2 }
135+
- match: { aggregations.path.properties.sort_values.2: 2.0 }
136+
137+
---
138+
"Test geo_line aggregation on geo points reverse sort by double and include_sort":
139+
- do:
140+
search:
141+
rest_total_hits_as_int: true
142+
index: locations
143+
size: 0
144+
body:
145+
aggs:
146+
path:
147+
geo_line:
148+
include_sort: true
149+
sort_order: "desc"
150+
point:
151+
field: location
152+
sort:
153+
field: rank
154+
- match: { hits.total: 3 }
155+
- match: { aggregations.path.type: "Feature" }
156+
- match: { aggregations.path.geometry.type: "LineString" }
157+
- length: { aggregations.path.geometry.coordinates: 3 }
158+
- match: { aggregations.path.geometry.coordinates.0.0: 13.371398 }
159+
- match: { aggregations.path.geometry.coordinates.0.1: 47.829303 }
160+
- match: { aggregations.path.geometry.coordinates.1.0: 13.37183 }
161+
- match: { aggregations.path.geometry.coordinates.1.1: 48.20842 }
162+
- match: { aggregations.path.geometry.coordinates.2.0: 13.378421 }
163+
- match: { aggregations.path.geometry.coordinates.2.1: 47.888321 }
164+
- is_true: aggregations.path.properties.complete
165+
- length: { aggregations.path.properties.sort_values: 3 }
166+
- match: { aggregations.path.properties.sort_values.0: 2.0 }
167+
- match: { aggregations.path.properties.sort_values.1: 1.2 }
168+
- match: { aggregations.path.properties.sort_values.2: 0.0 }
169+
170+
---
171+
"Test geo_line aggregation on geo points limit size":
172+
- do:
173+
search:
174+
rest_total_hits_as_int: true
175+
index: locations
176+
size: 0
177+
body:
178+
aggs:
179+
path:
180+
geo_line:
181+
size: 2
182+
point:
183+
field: location
184+
sort:
185+
field: rank
186+
- match: { hits.total: 3 }
187+
- match: { aggregations.path.type: "Feature" }
188+
- match: { aggregations.path.geometry.type: "LineString" }
189+
- length: { aggregations.path.geometry.coordinates: 2 }
190+
- match: { aggregations.path.geometry.coordinates.0.0: 13.378421 }
191+
- match: { aggregations.path.geometry.coordinates.0.1: 47.888321 }
192+
- match: { aggregations.path.geometry.coordinates.1.0: 13.37183 }
193+
- match: { aggregations.path.geometry.coordinates.1.1: 48.20842 }
194+
- is_false: aggregations.path.properties.complete
195+
196+
---
197+
"Test geo_line aggregation on geo points limit and sort":
198+
- do:
199+
search:
200+
rest_total_hits_as_int: true
201+
index: locations
202+
size: 0
203+
body:
204+
aggs:
205+
path:
206+
geo_line:
207+
size: 2
208+
include_sort: true
209+
sort_order: "desc"
210+
point:
211+
field: location
212+
sort:
213+
field: rank
214+
- match: { hits.total: 3 }
215+
- match: { aggregations.path.type: "Feature" }
216+
- match: { aggregations.path.geometry.type: "LineString" }
217+
- length: { aggregations.path.geometry.coordinates: 2 }
218+
- match: { aggregations.path.geometry.coordinates.0.0: 13.371398 }
219+
- match: { aggregations.path.geometry.coordinates.0.1: 47.829303 }
220+
- match: { aggregations.path.geometry.coordinates.1.0: 13.37183 }
221+
- match: { aggregations.path.geometry.coordinates.1.1: 48.20842 }
222+
- is_false: aggregations.path.properties.complete
223+
- length: { aggregations.path.properties.sort_values: 2 }
224+
- match: { aggregations.path.properties.sort_values.0: 2.0 }
225+
- match: { aggregations.path.properties.sort_values.1: 1.2 }
226+
82227
---
83228
"Test geo_line aggregation on geo points with no grouping":
84229
- do:

0 commit comments

Comments
 (0)