Skip to content

Commit fcccd07

Browse files
gashutosretabackslasht
authored and
shiv0408
committed
Enabling sort optimization back for half_float with custom comparators (opensearch-project#11024)
* Enabling sort optimizatin back for half_float with custom comparators Signed-off-by: Chaitanya Gohel <gashutos@amazon.com> * Fixing tests Signed-off-by: Chaitanya Gohel <gashutos@amazon.com> * Adding test for Indecx sort half_float Signed-off-by: Chaitanya Gohel <gashutos@amazon.com> * Making indexFieldData provate in FloatValuesComparatorSource Signed-off-by: Chaitanya Gohel <gashutos@amazon.com> * Update server/src/main/java/org/opensearch/index/search/comparators/HalfFloatComparator.java Co-authored-by: Andriy Redko <drreta@gmail.com> Signed-off-by: Chaitanya Gohel <104654647+gashutos@users.noreply.github.com> * Adding missing value instead null Signed-off-by: Chaitanya Gohel <gashutos@amazon.com> * Adding more tests for desc order sort Signed-off-by: Chaitanya Gohel <gashutos@amazon.com> * Update rest-api-spec/src/main/resources/rest-api-spec/test/search/90_search_after.yml Co-authored-by: Prabhakar Sithanandam <prabhakar.s87@gmail.com> Signed-off-by: Chaitanya Gohel <104654647+gashutos@users.noreply.github.com> * Adding tests in case missing values are competitive Signed-off-by: Chaitanya Gohel <gashutos@amazon.com> * chanheing newly added test supported version 3.0.0 Signed-off-by: Chaitanya Gohel <gashutos@amazon.com> * Assing missing float tests Signed-off-by: Chaitanya Gohel <gashutos@amazon.com> * Remove missing value change to be part of another PR Signed-off-by: Chaitanya Gohel <gashutos@amazon.com> --------- Signed-off-by: Chaitanya Gohel <gashutos@amazon.com> Signed-off-by: Chaitanya Gohel <104654647+gashutos@users.noreply.github.com> Co-authored-by: Andriy Redko <drreta@gmail.com> Co-authored-by: Prabhakar Sithanandam <prabhakar.s87@gmail.com> Signed-off-by: Shivansh Arora <hishiv@amazon.com>
1 parent 4c19be4 commit fcccd07

File tree

8 files changed

+483
-1
lines changed

8 files changed

+483
-1
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
2121
- [AdmissionControl] Added changes for AdmissionControl Interceptor and AdmissionControlService for RateLimiting ([#9286](https://github.com/opensearch-project/OpenSearch/pull/9286))
2222
- GHA to verify checklist items completion in PR descriptions ([#10800](https://github.com/opensearch-project/OpenSearch/pull/10800))
2323
- [Remote cluster state] Restore cluster state version during remote state auto restore ([#10853](https://github.com/opensearch-project/OpenSearch/pull/10853))
24+
- Add back half_float BKD based sort query optimization ([#11024](https://github.com/opensearch-project/OpenSearch/pull/11024))
2425

2526
### Dependencies
2627
- Bump `log4j-core` from 2.18.0 to 2.19.0

‎rest-api-spec/src/main/resources/rest-api-spec/test/indices.sort/10_basic.yml

+20
Original file line numberDiff line numberDiff line change
@@ -156,3 +156,23 @@
156156
query: {"range": { "rank": { "from": 0 } } }
157157
track_total_hits: false
158158
size: 3
159+
160+
---
161+
"Index Sort half float":
162+
- do:
163+
catch: bad_request
164+
indices.create:
165+
index: test
166+
body:
167+
settings:
168+
number_of_shards: 1
169+
number_of_replicas: 0
170+
index.sort.field: rank
171+
mappings:
172+
properties:
173+
rank:
174+
type: half_float
175+
176+
# This should failed with 400 as half_float is not supported for index sort
177+
- match: { status: 400 }
178+
- match: { error.type: illegal_argument_exception }

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

+85
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
properties:
2121
counter:
2222
type: double
23+
2324
- do:
2425
bulk:
2526
refresh: true
@@ -119,3 +120,87 @@
119120
- match: { status: 400 }
120121
- match: { error.type: search_phase_execution_exception }
121122
- match: { error.caused_by.reason: "Can't do sort across indices, as a field has [unsigned_long] type in one index, and different type in another index!" }
123+
124+
---
125+
"search across indices with mixed long and double and float numeric types":
126+
- skip:
127+
version: " - 2.10.99"
128+
reason: half float was broken before 2.11
129+
130+
- do:
131+
indices.create:
132+
index: test_1
133+
body:
134+
mappings:
135+
properties:
136+
counter:
137+
type: long
138+
139+
- do:
140+
indices.create:
141+
index: test_2
142+
body:
143+
mappings:
144+
properties:
145+
counter:
146+
type: double
147+
148+
- do:
149+
indices.create:
150+
index: test_3
151+
body:
152+
mappings:
153+
properties:
154+
counter:
155+
type: half_float
156+
157+
- do:
158+
bulk:
159+
refresh: true
160+
body:
161+
- index:
162+
_index: test_1
163+
- counter: 223372036854775800
164+
- index:
165+
_index: test_2
166+
- counter: 1223372036854775800.23
167+
- index:
168+
_index: test_2
169+
- counter: 184.4
170+
- index:
171+
_index: test_3
172+
- counter: 187.4
173+
- index:
174+
_index: test_3
175+
- counter: 194.4
176+
177+
- do:
178+
search:
179+
index: test_*
180+
rest_total_hits_as_int: true
181+
body:
182+
sort: [{ counter: desc }]
183+
- match: { hits.total: 5 }
184+
- length: { hits.hits: 5 }
185+
- match: { hits.hits.0._index: test_2 }
186+
- match: { hits.hits.0._source.counter: 1223372036854775800.23 }
187+
- match: { hits.hits.0.sort.0: 1223372036854775800.23 }
188+
- match: { hits.hits.1._index: test_1 }
189+
- match: { hits.hits.1._source.counter: 223372036854775800 }
190+
- match: { hits.hits.1.sort.0: 223372036854775800 }
191+
- match: { hits.hits.2._index: test_3 }
192+
- match: { hits.hits.2._source.counter: 194.4 }
193+
194+
- do:
195+
search:
196+
index: test_*
197+
rest_total_hits_as_int: true
198+
body:
199+
sort: [{ counter: asc }]
200+
- match: { hits.total: 5 }
201+
- length: { hits.hits: 5 }
202+
- match: { hits.hits.0._index: test_2 }
203+
- match: { hits.hits.0._source.counter: 184.4 }
204+
- match: { hits.hits.0.sort.0: 184.4 }
205+
- match: { hits.hits.1._index: test_3 }
206+
- match: { hits.hits.1._source.counter: 187.4 }

‎rest-api-spec/src/main/resources/rest-api-spec/test/search/90_search_after.yml

+127
Original file line numberDiff line numberDiff line change
@@ -320,3 +320,130 @@
320320
- length: { hits.hits: 1 }
321321
- match: { hits.hits.0._index: test }
322322
- match: { hits.hits.0._source.population: null }
323+
324+
---
325+
"half float":
326+
- skip:
327+
version: " - 2.10.99"
328+
reason: half_float was broken for 2.10 and earlier
329+
330+
- do:
331+
indices.create:
332+
index: test
333+
body:
334+
mappings:
335+
properties:
336+
population:
337+
type: half_float
338+
- do:
339+
bulk:
340+
refresh: true
341+
index: test
342+
body: |
343+
{"index":{}}
344+
{"population": 184.4}
345+
{"index":{}}
346+
{"population": 194.4}
347+
{"index":{}}
348+
{"population": 144.4}
349+
{"index":{}}
350+
{"population": 174.4}
351+
{"index":{}}
352+
{"population": 164.4}
353+
354+
- do:
355+
search:
356+
index: test
357+
rest_total_hits_as_int: true
358+
body:
359+
size: 3
360+
sort: [ { population: desc } ]
361+
- match: { hits.total: 5 }
362+
- length: { hits.hits: 3 }
363+
- match: { hits.hits.0._index: test }
364+
- match: { hits.hits.0._source.population: 194.4 }
365+
- match: { hits.hits.1._index: test }
366+
- match: { hits.hits.1._source.population: 184.4 }
367+
- match: { hits.hits.2._index: test }
368+
- match: { hits.hits.2._source.population: 174.4 }
369+
370+
- do:
371+
search:
372+
index: test
373+
rest_total_hits_as_int: true
374+
body:
375+
size: 3
376+
sort: [ { population: asc } ]
377+
- match: { hits.total: 5 }
378+
- length: { hits.hits: 3 }
379+
- match: { hits.hits.0._index: test }
380+
- match: { hits.hits.0._source.population: 144.4 }
381+
- match: { hits.hits.1._index: test }
382+
- match: { hits.hits.1._source.population: 164.4 }
383+
- match: { hits.hits.2._index: test }
384+
- match: { hits.hits.2._source.population: 174.4 }
385+
386+
# search_after with the asc sort
387+
- do:
388+
search:
389+
index: test
390+
rest_total_hits_as_int: true
391+
body:
392+
size: 1
393+
sort: [ { population: asc } ]
394+
search_after: [ 184.375 ] # this is rounded sort value in sort result
395+
- match: { hits.total: 5 }
396+
- length: { hits.hits: 1 }
397+
- match: { hits.hits.0._index: test }
398+
- match: { hits.hits.0._source.population: 194.4 }
399+
400+
# search_after with the desc sort
401+
- do:
402+
search:
403+
index: test
404+
rest_total_hits_as_int: true
405+
body:
406+
size: 1
407+
sort: [ { population: desc } ]
408+
search_after: [ 164.375 ] # this is rounded sort value in sort result
409+
- match: { hits.total: 5 }
410+
- length: { hits.hits: 1 }
411+
- match: { hits.hits.0._index: test }
412+
- match: { hits.hits.0._source.population: 144.4 }
413+
414+
# search_after with the asc sort with missing
415+
- do:
416+
bulk:
417+
refresh: true
418+
index: test
419+
body: |
420+
{"index":{}}
421+
{"population": null}
422+
- do:
423+
search:
424+
index: test
425+
rest_total_hits_as_int: true
426+
body:
427+
"size": 5
428+
"sort": [ { "population": { "order": "asc", "missing": "_last" } } ]
429+
search_after: [ 200 ] # making it out of min/max so only missing value hit is qualified
430+
431+
- match: { hits.total: 6 }
432+
- length: { hits.hits: 1 }
433+
- match: { hits.hits.0._index: test }
434+
- match: { hits.hits.0._source.population: null }
435+
436+
# search_after with the desc sort with missing
437+
- do:
438+
search:
439+
index: test
440+
rest_total_hits_as_int: true
441+
body:
442+
"size": 5
443+
"sort": [ { "population": { "order": "desc", "missing": "_last" } } ]
444+
search_after: [ 100 ] # making it out of min/max so only missing value hit is qualified
445+
446+
- match: { hits.total: 6 }
447+
- length: { hits.hits: 1 }
448+
- match: { hits.hits.0._index: test }
449+
- match: { hits.hits.0._source.population: null }

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

+60
Original file line numberDiff line numberDiff line change
@@ -605,6 +605,9 @@ public void testSimpleSorts() throws Exception {
605605
.startObject("float_value")
606606
.field("type", "float")
607607
.endObject()
608+
.startObject("half_float_value")
609+
.field("type", "half_float")
610+
.endObject()
608611
.startObject("double_value")
609612
.field("type", "double")
610613
.endObject()
@@ -628,6 +631,7 @@ public void testSimpleSorts() throws Exception {
628631
.field("long_value", i)
629632
.field("unsigned_long_value", UNSIGNED_LONG_BASE.add(BigInteger.valueOf(10000 * i)))
630633
.field("float_value", 0.1 * i)
634+
.field("half_float_value", 0.1 * i)
631635
.field("double_value", 0.1 * i)
632636
.endObject()
633637
);
@@ -794,6 +798,28 @@ public void testSimpleSorts() throws Exception {
794798

795799
assertThat(searchResponse.toString(), not(containsString("error")));
796800

801+
// HALF_FLOAT
802+
size = 1 + random.nextInt(10);
803+
searchResponse = client().prepareSearch().setQuery(matchAllQuery()).setSize(size).addSort("half_float_value", SortOrder.ASC).get();
804+
805+
assertHitCount(searchResponse, 10L);
806+
assertThat(searchResponse.getHits().getHits().length, equalTo(size));
807+
for (int i = 0; i < size; i++) {
808+
assertThat(searchResponse.getHits().getAt(i).getId(), equalTo(Integer.toString(i)));
809+
}
810+
811+
assertThat(searchResponse.toString(), not(containsString("error")));
812+
size = 1 + random.nextInt(10);
813+
searchResponse = client().prepareSearch().setQuery(matchAllQuery()).setSize(size).addSort("half_float_value", SortOrder.DESC).get();
814+
815+
assertHitCount(searchResponse, 10);
816+
assertThat(searchResponse.getHits().getHits().length, equalTo(size));
817+
for (int i = 0; i < size; i++) {
818+
assertThat(searchResponse.getHits().getAt(i).getId(), equalTo(Integer.toString(9 - i)));
819+
}
820+
821+
assertThat(searchResponse.toString(), not(containsString("error")));
822+
797823
// DOUBLE
798824
size = 1 + random.nextInt(10);
799825
searchResponse = client().prepareSearch().setQuery(matchAllQuery()).setSize(size).addSort("double_value", SortOrder.ASC).get();
@@ -1330,6 +1356,9 @@ public void testSortMVField() throws Exception {
13301356
.startObject("float_values")
13311357
.field("type", "float")
13321358
.endObject()
1359+
.startObject("half_float_values")
1360+
.field("type", "float")
1361+
.endObject()
13331362
.startObject("double_values")
13341363
.field("type", "double")
13351364
.endObject()
@@ -1351,6 +1380,7 @@ public void testSortMVField() throws Exception {
13511380
.array("short_values", 1, 5, 10, 8)
13521381
.array("byte_values", 1, 5, 10, 8)
13531382
.array("float_values", 1f, 5f, 10f, 8f)
1383+
.array("half_float_values", 1f, 5f, 10f, 8f)
13541384
.array("double_values", 1d, 5d, 10d, 8d)
13551385
.array("string_values", "01", "05", "10", "08")
13561386
.endObject()
@@ -1365,6 +1395,7 @@ public void testSortMVField() throws Exception {
13651395
.array("short_values", 11, 15, 20, 7)
13661396
.array("byte_values", 11, 15, 20, 7)
13671397
.array("float_values", 11f, 15f, 20f, 7f)
1398+
.array("half_float_values", 11f, 15f, 20f, 7f)
13681399
.array("double_values", 11d, 15d, 20d, 7d)
13691400
.array("string_values", "11", "15", "20", "07")
13701401
.endObject()
@@ -1379,6 +1410,7 @@ public void testSortMVField() throws Exception {
13791410
.array("short_values", 2, 1, 3, -4)
13801411
.array("byte_values", 2, 1, 3, -4)
13811412
.array("float_values", 2f, 1f, 3f, -4f)
1413+
.array("half_float_values", 2f, 1f, 3f, -4f)
13821414
.array("double_values", 2d, 1d, 3d, -4d)
13831415
.array("string_values", "02", "01", "03", "!4")
13841416
.endObject()
@@ -1585,6 +1617,34 @@ public void testSortMVField() throws Exception {
15851617
assertThat(searchResponse.getHits().getAt(2).getId(), equalTo(Integer.toString(3)));
15861618
assertThat(((Number) searchResponse.getHits().getAt(2).getSortValues()[0]).floatValue(), equalTo(3f));
15871619

1620+
searchResponse = client().prepareSearch().setQuery(matchAllQuery()).setSize(10).addSort("half_float_values", SortOrder.ASC).get();
1621+
1622+
assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L));
1623+
assertThat(searchResponse.getHits().getHits().length, equalTo(3));
1624+
1625+
assertThat(searchResponse.getHits().getAt(0).getId(), equalTo(Integer.toString(3)));
1626+
assertThat(((Number) searchResponse.getHits().getAt(0).getSortValues()[0]).floatValue(), equalTo(-4f));
1627+
1628+
assertThat(searchResponse.getHits().getAt(1).getId(), equalTo(Integer.toString(1)));
1629+
assertThat(((Number) searchResponse.getHits().getAt(1).getSortValues()[0]).floatValue(), equalTo(1f));
1630+
1631+
assertThat(searchResponse.getHits().getAt(2).getId(), equalTo(Integer.toString(2)));
1632+
assertThat(((Number) searchResponse.getHits().getAt(2).getSortValues()[0]).floatValue(), equalTo(7f));
1633+
1634+
searchResponse = client().prepareSearch().setQuery(matchAllQuery()).setSize(10).addSort("half_float_values", SortOrder.DESC).get();
1635+
1636+
assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L));
1637+
assertThat(searchResponse.getHits().getHits().length, equalTo(3));
1638+
1639+
assertThat(searchResponse.getHits().getAt(0).getId(), equalTo(Integer.toString(2)));
1640+
assertThat(((Number) searchResponse.getHits().getAt(0).getSortValues()[0]).floatValue(), equalTo(20f));
1641+
1642+
assertThat(searchResponse.getHits().getAt(1).getId(), equalTo(Integer.toString(1)));
1643+
assertThat(((Number) searchResponse.getHits().getAt(1).getSortValues()[0]).floatValue(), equalTo(10f));
1644+
1645+
assertThat(searchResponse.getHits().getAt(2).getId(), equalTo(Integer.toString(3)));
1646+
assertThat(((Number) searchResponse.getHits().getAt(2).getSortValues()[0]).floatValue(), equalTo(3f));
1647+
15881648
searchResponse = client().prepareSearch().setQuery(matchAllQuery()).setSize(10).addSort("double_values", SortOrder.ASC).get();
15891649

15901650
assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L));

0 commit comments

Comments
 (0)