Skip to content

Commit 1072093

Browse files
bugmakerrrrrrakolarkunnu
bugmakerrrrrr
authored and
akolarkunnu
committed
use the correct type to widen the sort fields when merging top docs (opensearch-project#16881)
* use the correct type to widen the sort fields when merging top docs Signed-off-by: panguixin <[email protected]> * fix Signed-off-by: panguixin <[email protected]> * apply commments Signed-off-by: panguixin <[email protected]> * changelog Signed-off-by: panguixin <[email protected]> * add more tests Signed-off-by: panguixin <[email protected]> --------- Signed-off-by: panguixin <[email protected]>
1 parent 4e50136 commit 1072093

File tree

4 files changed

+152
-23
lines changed

4 files changed

+152
-23
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
6666
- Indexed IP field supports `terms_query` with more than 1025 IP masks [#16391](https://github.com/opensearch-project/OpenSearch/pull/16391)
6767
- Make entries for dependencies from server/build.gradle to gradle version catalog ([#16707](https://github.com/opensearch-project/OpenSearch/pull/16707))
6868
- Allow extended plugins to be optional ([#16909](https://github.com/opensearch-project/OpenSearch/pull/16909))
69+
- Use the correct type to widen the sort fields when merging top docs ([#16881](https://github.com/opensearch-project/OpenSearch/pull/16881))
6970

7071
### Deprecated
7172
- Performing update operation with default pipeline or final pipeline is deprecated ([#16712](https://github.com/opensearch-project/OpenSearch/pull/16712))

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

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import org.opensearch.common.Numbers;
5050
import org.opensearch.common.settings.Settings;
5151
import org.opensearch.common.xcontent.XContentFactory;
52+
import org.opensearch.common.xcontent.XContentType;
5253
import org.opensearch.core.rest.RestStatus;
5354
import org.opensearch.core.xcontent.MediaTypeRegistry;
5455
import org.opensearch.core.xcontent.XContentBuilder;
@@ -63,6 +64,7 @@
6364
import org.opensearch.search.SearchHit;
6465
import org.opensearch.search.SearchHits;
6566
import org.opensearch.test.InternalSettingsPlugin;
67+
import org.opensearch.test.OpenSearchTestCase;
6668
import org.opensearch.test.ParameterizedDynamicSettingsOpenSearchIntegTestCase;
6769
import org.hamcrest.Matchers;
6870

@@ -82,7 +84,9 @@
8284
import java.util.Set;
8385
import java.util.TreeMap;
8486
import java.util.concurrent.ExecutionException;
87+
import java.util.concurrent.atomic.AtomicInteger;
8588
import java.util.function.Function;
89+
import java.util.function.Supplier;
8690

8791
import static org.opensearch.common.xcontent.XContentFactory.jsonBuilder;
8892
import static org.opensearch.index.query.QueryBuilders.functionScoreQuery;
@@ -2609,4 +2613,99 @@ public void testSimpleSortsPoints() throws Exception {
26092613

26102614
assertThat(searchResponse.toString(), not(containsString("error")));
26112615
}
2616+
2617+
public void testSortMixedIntegerNumericFields() throws Exception {
2618+
internalCluster().ensureAtLeastNumDataNodes(3);
2619+
AtomicInteger counter = new AtomicInteger();
2620+
index("long", () -> Long.MAX_VALUE - counter.getAndIncrement());
2621+
index("integer", () -> Integer.MAX_VALUE - counter.getAndIncrement());
2622+
SearchResponse searchResponse = client().prepareSearch("long", "integer")
2623+
.setQuery(matchAllQuery())
2624+
.setSize(10)
2625+
.addSort(SortBuilders.fieldSort("field").order(SortOrder.ASC).sortMode(SortMode.MAX))
2626+
.get();
2627+
assertNoFailures(searchResponse);
2628+
long[] sortValues = new long[10];
2629+
for (int i = 0; i < 10; i++) {
2630+
sortValues[i] = ((Number) searchResponse.getHits().getAt(i).getSortValues()[0]).longValue();
2631+
}
2632+
for (int i = 1; i < 10; i++) {
2633+
assertThat(Arrays.toString(sortValues), sortValues[i - 1], lessThan(sortValues[i]));
2634+
}
2635+
}
2636+
2637+
public void testSortMixedFloatingNumericFields() throws Exception {
2638+
internalCluster().ensureAtLeastNumDataNodes(3);
2639+
AtomicInteger counter = new AtomicInteger();
2640+
index("double", () -> 100.5 - counter.getAndIncrement());
2641+
counter.set(0);
2642+
index("float", () -> 200.5 - counter.getAndIncrement());
2643+
counter.set(0);
2644+
index("half_float", () -> 300.5 - counter.getAndIncrement());
2645+
SearchResponse searchResponse = client().prepareSearch("double", "float", "half_float")
2646+
.setQuery(matchAllQuery())
2647+
.setSize(15)
2648+
.addSort(SortBuilders.fieldSort("field").order(SortOrder.ASC).sortMode(SortMode.MAX))
2649+
.get();
2650+
assertNoFailures(searchResponse);
2651+
double[] sortValues = new double[15];
2652+
for (int i = 0; i < 15; i++) {
2653+
sortValues[i] = ((Number) searchResponse.getHits().getAt(i).getSortValues()[0]).doubleValue();
2654+
}
2655+
for (int i = 1; i < 15; i++) {
2656+
assertThat(Arrays.toString(sortValues), sortValues[i - 1], lessThan(sortValues[i]));
2657+
}
2658+
}
2659+
2660+
public void testSortMixedFloatingAndIntegerNumericFields() throws Exception {
2661+
internalCluster().ensureAtLeastNumDataNodes(3);
2662+
index("long", () -> randomLongBetween(0, (long) 2E53 - 1));
2663+
index("integer", OpenSearchTestCase::randomInt);
2664+
index("double", OpenSearchTestCase::randomDouble);
2665+
index("float", () -> randomFloat());
2666+
boolean asc = randomBoolean();
2667+
SearchResponse searchResponse = client().prepareSearch("long", "integer", "double", "float")
2668+
.setQuery(matchAllQuery())
2669+
.setSize(20)
2670+
.addSort(SortBuilders.fieldSort("field").order(asc ? SortOrder.ASC : SortOrder.DESC).sortMode(SortMode.MAX))
2671+
.get();
2672+
assertNoFailures(searchResponse);
2673+
double[] sortValues = new double[20];
2674+
for (int i = 0; i < 20; i++) {
2675+
sortValues[i] = ((Number) searchResponse.getHits().getAt(i).getSortValues()[0]).doubleValue();
2676+
}
2677+
if (asc) {
2678+
for (int i = 1; i < 20; i++) {
2679+
assertThat(Arrays.toString(sortValues), sortValues[i - 1], lessThanOrEqualTo(sortValues[i]));
2680+
}
2681+
} else {
2682+
for (int i = 1; i < 20; i++) {
2683+
assertThat(Arrays.toString(sortValues), sortValues[i - 1], greaterThanOrEqualTo(sortValues[i]));
2684+
}
2685+
}
2686+
}
2687+
2688+
private void index(String type, Supplier<Number> valueSupplier) throws Exception {
2689+
assertAcked(
2690+
prepareCreate(type).setMapping(
2691+
XContentFactory.jsonBuilder()
2692+
.startObject()
2693+
.startObject("properties")
2694+
.startObject("field")
2695+
.field("type", type)
2696+
.endObject()
2697+
.endObject()
2698+
.endObject()
2699+
).setSettings(Settings.builder().put("index.number_of_shards", 3).put("index.number_of_replicas", 0))
2700+
);
2701+
ensureGreen(type);
2702+
for (int i = 0; i < 5; i++) {
2703+
client().prepareIndex(type)
2704+
.setId(Integer.toString(i))
2705+
.setSource("{\"field\" : " + valueSupplier.get() + " }", XContentType.JSON)
2706+
.get();
2707+
}
2708+
client().admin().indices().prepareRefresh(type).get();
2709+
}
2710+
26122711
}

‎server/src/main/java/org/opensearch/action/search/SearchPhaseController.java‎

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import org.opensearch.common.lucene.search.TopDocsAndMaxScore;
4949
import org.opensearch.core.common.breaker.CircuitBreaker;
5050
import org.opensearch.core.common.io.stream.NamedWriteableRegistry;
51+
import org.opensearch.index.fielddata.IndexFieldData;
5152
import org.opensearch.search.DocValueFormat;
5253
import org.opensearch.search.SearchHit;
5354
import org.opensearch.search.SearchHits;
@@ -604,36 +605,51 @@ private static void validateMergeSortValueFormats(Collection<? extends SearchPha
604605
* support sort optimization, we removed type widening there and taking care here during merging.
605606
* More details here https://github.com/opensearch-project/OpenSearch/issues/6326
606607
*/
608+
// TODO: should we check the compatibility between types
607609
private static Sort createSort(TopFieldDocs[] topFieldDocs) {
608610
final SortField[] firstTopDocFields = topFieldDocs[0].fields;
609611
final SortField[] newFields = new SortField[firstTopDocFields.length];
612+
for (int fieldIndex = 0; fieldIndex < firstTopDocFields.length; fieldIndex++) {
613+
SortField.Type firstType = getSortType(firstTopDocFields[fieldIndex]);
614+
newFields[fieldIndex] = firstTopDocFields[fieldIndex];
615+
if (SortedWiderNumericSortField.isTypeSupported(firstType) == false) {
616+
continue;
617+
}
610618

611-
for (int i = 0; i < firstTopDocFields.length; i++) {
612-
final SortField delegate = firstTopDocFields[i];
613-
final SortField.Type type = delegate instanceof SortedNumericSortField
614-
? ((SortedNumericSortField) delegate).getNumericType()
615-
: delegate.getType();
619+
boolean requireWiden = false;
620+
boolean isFloat = firstType == SortField.Type.FLOAT || firstType == SortField.Type.DOUBLE;
621+
for (int shardIndex = 1; shardIndex < topFieldDocs.length; shardIndex++) {
622+
final SortField sortField = topFieldDocs[shardIndex].fields[fieldIndex];
623+
SortField.Type sortType = getSortType(sortField);
624+
if (SortedWiderNumericSortField.isTypeSupported(sortType) == false) {
625+
// throw exception if sortType is not CUSTOM?
626+
// skip this shard or do not widen?
627+
requireWiden = false;
628+
break;
629+
}
630+
requireWiden = requireWiden || sortType != firstType;
631+
isFloat = isFloat || sortType == SortField.Type.FLOAT || sortType == SortField.Type.DOUBLE;
632+
}
616633

617-
if (SortedWiderNumericSortField.isTypeSupported(type) && isSortWideningRequired(topFieldDocs, i)) {
618-
newFields[i] = new SortedWiderNumericSortField(delegate.getField(), type, delegate.getReverse());
619-
} else {
620-
newFields[i] = firstTopDocFields[i];
634+
if (requireWiden) {
635+
newFields[fieldIndex] = new SortedWiderNumericSortField(
636+
firstTopDocFields[fieldIndex].getField(),
637+
isFloat ? SortField.Type.DOUBLE : SortField.Type.LONG,
638+
firstTopDocFields[fieldIndex].getReverse()
639+
);
621640
}
622641
}
623642
return new Sort(newFields);
624643
}
625644

626-
/**
627-
* It will compare respective SortField between shards to see if any shard results have different
628-
* field mapping type, accordingly it will decide to widen the sort fields.
629-
*/
630-
private static boolean isSortWideningRequired(TopFieldDocs[] topFieldDocs, int sortFieldindex) {
631-
for (int i = 0; i < topFieldDocs.length - 1; i++) {
632-
if (!topFieldDocs[i].fields[sortFieldindex].equals(topFieldDocs[i + 1].fields[sortFieldindex])) {
633-
return true;
634-
}
645+
private static SortField.Type getSortType(SortField sortField) {
646+
if (sortField.getComparatorSource() instanceof IndexFieldData.XFieldComparatorSource) {
647+
return ((IndexFieldData.XFieldComparatorSource) sortField.getComparatorSource()).reducedType();
648+
} else {
649+
return sortField instanceof SortedNumericSortField
650+
? ((SortedNumericSortField) sortField).getNumericType()
651+
: sortField.getType();
635652
}
636-
return false;
637653
}
638654

639655
/*

‎server/src/main/java/org/opensearch/search/sort/SortedWiderNumericSortField.java‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,18 @@
2121
import org.apache.lucene.search.comparators.NumericComparator;
2222

2323
import java.io.IOException;
24+
import java.util.Comparator;
2425

2526
/**
26-
* Sorted numeric field for wider sort types,
27-
* to help sorting two different numeric types.
27+
* Sorted numeric field for wider sort types, to help sorting two different numeric types.
28+
* NOTE: the unsigned_long is not supported by widening sort since the unsigned_long could not be used with other types
2829
*
2930
* @opensearch.internal
3031
*/
3132
public class SortedWiderNumericSortField extends SortedNumericSortField {
33+
private final int byteCounts;
34+
private final Comparator<Number> comparator;
35+
3236
/**
3337
* Creates a sort, possibly in reverse, specifying how the sort value from the document's set is
3438
* selected.
@@ -39,6 +43,15 @@ public class SortedWiderNumericSortField extends SortedNumericSortField {
3943
*/
4044
public SortedWiderNumericSortField(String field, Type type, boolean reverse) {
4145
super(field, type, reverse);
46+
if (type == Type.LONG) {
47+
byteCounts = Long.BYTES;
48+
comparator = Comparator.comparingLong(Number::longValue);
49+
} else if (type == Type.DOUBLE) {
50+
byteCounts = Double.BYTES;
51+
comparator = Comparator.comparingDouble(Number::doubleValue);
52+
} else {
53+
throw new IllegalArgumentException("Unsupported numeric type: " + type);
54+
}
4255
}
4356

4457
/**
@@ -51,7 +64,7 @@ public SortedWiderNumericSortField(String field, Type type, boolean reverse) {
5164
*/
5265
@Override
5366
public FieldComparator<?> getComparator(int numHits, Pruning pruning) {
54-
return new NumericComparator<Number>(getField(), (Number) getMissingValue(), getReverse(), pruning, Double.BYTES) {
67+
return new NumericComparator<Number>(getField(), (Number) getMissingValue(), getReverse(), pruning, byteCounts) {
5568
@Override
5669
public int compare(int slot1, int slot2) {
5770
throw new UnsupportedOperationException();
@@ -78,7 +91,7 @@ public int compareValues(Number first, Number second) {
7891
} else if (second == null) {
7992
return 1;
8093
} else {
81-
return Double.compare(first.doubleValue(), second.doubleValue());
94+
return comparator.compare(first, second);
8295
}
8396
}
8497
};

0 commit comments

Comments
 (0)