Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use the correct type to widen the sort fields when merging top docs #16881

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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();
}
}
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,53 @@
* 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?
// skip this shard or do not widen?
requireWiden = false;
break;

Check warning on line 628 in server/src/main/java/org/opensearch/action/search/SearchPhaseController.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/action/search/SearchPhaseController.java#L627-L628

Added lines #L627 - L628 were not covered by tests
}
requireWiden = requireWiden || sortType != firstType;
isFloat = 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();

Check warning on line 649 in server/src/main/java/org/opensearch/action/search/SearchPhaseController.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/action/search/SearchPhaseController.java#L647-L649

Added lines #L647 - L649 were not covered by tests
} else {
return sortField instanceof SortedNumericSortField
? ((SortedNumericSortField) sortField).getNumericType()
: sortField.getType();
}
return false;
}

/*
Expand Down
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
Loading