Skip to content

Commit

Permalink
use the correct type to widen the sort fields when merging top docs
Browse files Browse the repository at this point in the history 
Signed-off-by: panguixin <[email protected]>
  • Loading branch information
@bugmakerrrrrr
bugmakerrrrrr committed Dec 18, 2024
1 parent b5f651f commit 219f48d
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 21 deletions.
40 changes: 40 additions & 0 deletions server/src/internalClusterTest/java/org/opensearch/search/sort/FieldSortIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import org.opensearch.common.Numbers;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.xcontent.XContentFactory;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.core.xcontent.MediaTypeRegistry;
import org.opensearch.core.xcontent.XContentBuilder;
Expand Down Expand Up @@ -2609,4 +2610,43 @@ public void testSimpleSortsPoints() throws Exception {

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

public void testSortMixedNumericFields() throws Exception {
internalCluster().ensureAtLeastNumDataNodes(3);
index("long", Long.MAX_VALUE);
index("integer", Integer.MAX_VALUE);
SearchResponse searchResponse = client().prepareSearch("long", "integer")
.setQuery(matchAllQuery())
.setSize(10)
.addSort(SortBuilders.fieldSort("field").order(SortOrder.ASC).sortMode(SortMode.MAX))
.get();
assertNoFailures(searchResponse);
long[] sortValues = new long[10];
for (int i = 0; i < 10; i++) {
sortValues[i] = ((Number) searchResponse.getHits().getAt(i).getSortValues()[0]).longValue();
}
for (int i = 1; i < 10; i++) {
assertThat(Arrays.toString(sortValues), sortValues[i - 1], lessThan(sortValues[i]));
}
}

private void index(String type, long end) throws Exception {
assertAcked(
prepareCreate(type).setMapping(
XContentFactory.jsonBuilder()
.startObject()
.startObject("properties")
.startObject("field")
.field("type", type)
.endObject()
.endObject()
.endObject()
).setSettings(Settings.builder().put("index.number_of_shards", 3).put("index.number_of_replicas", 0))
);
ensureGreen(type);
for (int i = 0; i < 5; i++) {
client().prepareIndex(type).setId(Integer.toString(i)).setSource("{\"field\" : " + (end - i) + " }", XContentType.JSON).get();
}
client().admin().indices().prepareRefresh(type).get();
}
}
55 changes: 36 additions & 19 deletions server/src/main/java/org/opensearch/action/search/SearchPhaseController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import org.opensearch.common.lucene.search.TopDocsAndMaxScore;
import org.opensearch.core.common.breaker.CircuitBreaker;
import org.opensearch.core.common.io.stream.NamedWriteableRegistry;
import org.opensearch.index.fielddata.IndexFieldData;
import org.opensearch.search.DocValueFormat;
import org.opensearch.search.SearchHit;
import org.opensearch.search.SearchHits;
Expand Down Expand Up @@ -604,36 +605,52 @@ private static void validateMergeSortValueFormats(Collection<? extends SearchPha
* support sort optimization, we removed type widening there and taking care here during merging.
* More details here https://github.com/opensearch-project/OpenSearch/issues/6326
*/
// TODO: should we check the compatibility between types
private static Sort createSort(TopFieldDocs[] topFieldDocs) {
final SortField[] firstTopDocFields = topFieldDocs[0].fields;
final SortField[] newFields = new SortField[firstTopDocFields.length];
for (int fieldIndex = 0; fieldIndex < firstTopDocFields.length; fieldIndex++) {
SortField.Type firstType = getSortType(firstTopDocFields[fieldIndex]);
newFields[fieldIndex] = firstTopDocFields[fieldIndex];
if (SortedWiderNumericSortField.isTypeSupported(firstType) == false) {
continue;
}

for (int i = 0; i < firstTopDocFields.length; i++) {
final SortField delegate = firstTopDocFields[i];
final SortField.Type type = delegate instanceof SortedNumericSortField
? ((SortedNumericSortField) delegate).getNumericType()
: delegate.getType();
boolean requireWiden = false;
boolean isFloat = firstType == SortField.Type.FLOAT || firstType == SortField.Type.DOUBLE;
for (int shardIndex = 1; shardIndex < topFieldDocs.length; shardIndex++) {
final SortField sortField = topFieldDocs[shardIndex].fields[fieldIndex];
SortField.Type sortType = getSortType(sortField);
if (SortedWiderNumericSortField.isTypeSupported(sortType) == false) {
// throw exception if sortType is not CUSTOM?
requireWiden = false;
break;
}
requireWiden = sortType != firstType;
isFloat = sortType == SortField.Type.FLOAT || sortType == SortField.Type.DOUBLE;
}

if (SortedWiderNumericSortField.isTypeSupported(type) && isSortWideningRequired(topFieldDocs, i)) {
newFields[i] = new SortedWiderNumericSortField(delegate.getField(), type, delegate.getReverse());
} else {
newFields[i] = firstTopDocFields[i];
if (requireWiden) {
newFields[fieldIndex] = new SortedWiderNumericSortField(
firstTopDocFields[fieldIndex].getField(),
isFloat ? SortField.Type.DOUBLE : SortField.Type.LONG,
firstTopDocFields[fieldIndex].getReverse()
);
}
}
return new Sort(newFields);
}

/**
* It will compare respective SortField between shards to see if any shard results have different
* field mapping type, accordingly it will decide to widen the sort fields.
*/
private static boolean isSortWideningRequired(TopFieldDocs[] topFieldDocs, int sortFieldindex) {
for (int i = 0; i < topFieldDocs.length - 1; i++) {
if (!topFieldDocs[i].fields[sortFieldindex].equals(topFieldDocs[i + 1].fields[sortFieldindex])) {
return true;
}
private static SortField.Type getSortType(SortField sortField) {
if (sortField.getComparatorSource() != null) {
IndexFieldData.XFieldComparatorSource comparatorSource = (IndexFieldData.XFieldComparatorSource) sortField
.getComparatorSource();
return comparatorSource.reducedType();
} else {
return sortField instanceof SortedNumericSortField
? ((SortedNumericSortField) sortField).getNumericType()
: sortField.getType();
}
return false;
}

/*
Expand Down
10 changes: 8 additions & 2 deletions server/src/main/java/org/opensearch/search/sort/SortedWiderNumericSortField.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.apache.lucene.search.comparators.NumericComparator;

import java.io.IOException;
import java.util.Comparator;

/**
* Sorted numeric field for wider sort types,
Expand All @@ -29,6 +30,9 @@
* @opensearch.internal
*/
public class SortedWiderNumericSortField extends SortedNumericSortField {
private final int byteCounts;
private final Comparator<Number> comparator;

/**
* Creates a sort, possibly in reverse, specifying how the sort value from the document's set is
* selected.
Expand All @@ -39,6 +43,8 @@ public class SortedWiderNumericSortField extends SortedNumericSortField {
*/
public SortedWiderNumericSortField(String field, Type type, boolean reverse) {
super(field, type, reverse);
byteCounts = type == Type.LONG ? Long.BYTES : Double.BYTES;
comparator = type == Type.LONG ? Comparator.comparingLong(Number::longValue) : Comparator.comparingDouble(Number::doubleValue);
}

/**
Expand All @@ -51,7 +57,7 @@ public SortedWiderNumericSortField(String field, Type type, boolean reverse) {
*/
@Override
public FieldComparator<?> getComparator(int numHits, Pruning pruning) {
return new NumericComparator<Number>(getField(), (Number) getMissingValue(), getReverse(), pruning, Double.BYTES) {
return new NumericComparator<Number>(getField(), (Number) getMissingValue(), getReverse(), pruning, byteCounts) {
@Override
public int compare(int slot1, int slot2) {
throw new UnsupportedOperationException();
Expand All @@ -78,7 +84,7 @@ public int compareValues(Number first, Number second) {
} else if (second == null) {
return 1;
} else {
return Double.compare(first.doubleValue(), second.doubleValue());
return comparator.compare(first, second);
}
}
};
Expand Down

0 comments on commit 219f48d

Please sign in to comment.