Skip to content

Commit ef969b2

Browse files
ajleong623owaiskazi19
authored and
vinaykpud
committed
Null field when getting point values for geopoint sort (opensearch-project#18843)
* null catch Signed-off-by: Anthony Leong <aj.leong623@gmail.com> * added tests for code coverage Signed-off-by: Anthony Leong <aj.leong623@gmail.com> * Update server/src/main/java/org/opensearch/search/internal/ExitableDirectoryReader.java Co-authored-by: Owais Kazi <owaiskazi19@gmail.com> Signed-off-by: Anthony Leong <aj.leong623@gmail.com> * empty string check Signed-off-by: Anthony Leong <aj.leong623@gmail.com> * add yaml test Signed-off-by: Anthony Leong <aj.leong623@gmail.com> * add extra test Signed-off-by: Anthony Leong <aj.leong623@gmail.com> * spotless apply Signed-off-by: Anthony Leong <aj.leong623@gmail.com> * random change to rerun workflow Signed-off-by: Anthony Leong <aj.leong623@gmail.com> * rerun workflow Signed-off-by: Anthony Leong <aj.leong623@gmail.com> * added getPointValues unit test Signed-off-by: Anthony Leong <aj.leong623@gmail.com> * non-deterministic tests Signed-off-by: Anthony Leong <aj.leong623@gmail.com> --------- Signed-off-by: Anthony Leong <aj.leong623@gmail.com> Co-authored-by: Owais Kazi <owaiskazi19@gmail.com>
1 parent fe61ff2 commit ef969b2

File tree

4 files changed

+100
-0
lines changed

4 files changed

+100
-0
lines changed

‎rest-api-spec/src/main/resources/rest-api-spec/test/search/260_sort_geopoint.yml‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
setup:
2+
- do:
3+
indices.create:
4+
index: index
5+
body:
6+
mappings:
7+
properties:
8+
admin:
9+
type: keyword
10+
location:
11+
type: geo_point
12+
- do:
13+
index:
14+
index: index
15+
refresh: true
16+
body: {admin: "11", location: {lat: 48.8331, lon: 2.3264}}
17+
18+
---
19+
"test query then sort with geo_point distance":
20+
21+
- do:
22+
search:
23+
index: index
24+
body:
25+
query:
26+
exists:
27+
field: location
28+
sort: [{_geo_distance: {location: {lat: 40.7128, lon: -74.0060}, ignore_unmapped: true, order: "desc", unit: "km"}}]
29+
size: 4
30+
- match: { hits.total.value: 1 }
31+
32+
- do:
33+
search:
34+
index: index
35+
body:
36+
query: { match_all: {} }
37+
sort: [{_geo_distance: {location: [9.227400, 49.189800], order: "desc", unit: "km", distance_type: "arc", mode: "min", ignore_unmapped: true } } ]
38+
size: 10
39+
- match: { hits.total.value: 1 }

‎server/src/internalClusterTest/java/org/opensearch/search/sort/GeoDistanceSortBuilderIT.java‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,9 @@
4242
import org.opensearch.common.settings.Settings;
4343
import org.opensearch.common.unit.DistanceUnit;
4444
import org.opensearch.core.xcontent.XContentBuilder;
45+
import org.opensearch.index.query.BoolQueryBuilder;
4546
import org.opensearch.index.query.GeoValidationMethod;
47+
import org.opensearch.index.query.MatchAllQueryBuilder;
4648
import org.opensearch.search.builder.SearchSourceBuilder;
4749
import org.opensearch.test.ParameterizedStaticSettingsOpenSearchIntegTestCase;
4850
import org.opensearch.test.VersionUtils;
@@ -56,6 +58,8 @@
5658
import java.util.concurrent.ExecutionException;
5759

5860
import static org.opensearch.common.xcontent.XContentFactory.jsonBuilder;
61+
import static org.opensearch.index.query.QueryBuilders.boolQuery;
62+
import static org.opensearch.index.query.QueryBuilders.existsQuery;
5963
import static org.opensearch.index.query.QueryBuilders.matchAllQuery;
6064
import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING;
6165
import static org.opensearch.search.sort.SortBuilders.fieldSort;
@@ -430,4 +434,56 @@ public void testCrossIndexIgnoreUnmapped() throws Exception {
430434
new Object[] { Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY }
431435
);
432436
}
437+
438+
public void testGeoDistanceQueryThenSort() throws Exception {
439+
assertAcked(prepareCreate("index").setMapping("admin", "type=keyword", LOCATION_FIELD, "type=geo_point"));
440+
441+
indexRandom(
442+
true,
443+
client().prepareIndex("index")
444+
.setId("d1")
445+
.setSource(
446+
jsonBuilder().startObject()
447+
.startObject(LOCATION_FIELD)
448+
.field("lat", 48.8331)
449+
.field("lon", 2.3264)
450+
.endObject()
451+
.field("admin", "11")
452+
.endObject()
453+
)
454+
);
455+
456+
GeoDistanceSortBuilder geoDistanceSortBuilder = new GeoDistanceSortBuilder(LOCATION_FIELD, new GeoPoint(40.7128, -74.0060));
457+
458+
BoolQueryBuilder bool = boolQuery().filter(existsQuery(LOCATION_FIELD));
459+
460+
SearchResponse searchResponse = client().prepareSearch()
461+
.setQuery(bool)
462+
.addSort(geoDistanceSortBuilder.unit(DistanceUnit.KILOMETERS).ignoreUnmapped(true).order(SortOrder.DESC))
463+
.setSize(4)
464+
.get();
465+
assertOrderedSearchHits(searchResponse, "d1");
466+
assertThat(
467+
(Double) searchResponse.getHits().getAt(0).getSortValues()[0],
468+
closeTo(GeoDistance.ARC.calculate(40.7128, -74.0060, 48.8331, 2.3264, DistanceUnit.KILOMETERS), 1.e-1)
469+
);
470+
471+
geoDistanceSortBuilder = new GeoDistanceSortBuilder(LOCATION_FIELD, new GeoPoint(9.227400, 49.189800));
472+
searchResponse = client().prepareSearch()
473+
.setQuery(new MatchAllQueryBuilder())
474+
.addSort(
475+
geoDistanceSortBuilder.unit(DistanceUnit.KILOMETERS)
476+
.ignoreUnmapped(true)
477+
.order(SortOrder.DESC)
478+
.geoDistance(GeoDistance.ARC)
479+
.sortMode(SortMode.MIN)
480+
)
481+
.setSize(10)
482+
.get();
483+
assertOrderedSearchHits(searchResponse, "d1");
484+
assertThat(
485+
(Double) searchResponse.getHits().getAt(0).getSortValues()[0],
486+
closeTo(GeoDistance.ARC.calculate(9.227400, 49.189800, 48.8331, 2.3264, DistanceUnit.KILOMETERS), 1.e-1)
487+
);
488+
}
433489
}

‎server/src/main/java/org/opensearch/search/internal/ExitableDirectoryReader.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
import org.apache.lucene.util.BytesRef;
4646
import org.apache.lucene.util.automaton.CompiledAutomaton;
4747
import org.opensearch.common.lucene.index.SequentialStoredFieldsLeafReader;
48+
import org.opensearch.core.common.Strings;
4849

4950
import java.io.IOException;
5051

@@ -108,6 +109,9 @@ private ExitableLeafReader(LeafReader leafReader, QueryCancellation queryCancell
108109

109110
@Override
110111
public PointValues getPointValues(String field) throws IOException {
112+
if (Strings.isEmpty(field)) {
113+
return null;
114+
}
111115
final PointValues pointValues = in.getPointValues(field);
112116
if (pointValues == null) {
113117
return null;

‎server/src/test/java/org/opensearch/search/SearchCancellationTests.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,7 @@ public void testExitableDirectoryReader() throws IOException {
201201
);
202202

203203
cancelled.set(false); // Avoid exception during construction of the wrapper objects
204+
assertNull(searcher.getIndexReader().leaves().get(0).reader().getPointValues(null));
204205
Terms terms = searcher.getIndexReader().leaves().get(0).reader().terms(STRING_FIELD_NAME);
205206
TermsEnum termsIterator = terms.iterator();
206207
TermsEnum termsIntersect = terms.intersect(automaton, null);

0 commit comments

Comments
 (0)