Skip to content

Commit ffa67f9

Browse files
bugmakerrrrrrbugmakerrrrrr
authored
Fix missing value of FieldSort for unsigned_long (#14963)
* Fix missing value of FieldSort for unsigned_long Signed-off-by: panguixin <[email protected]> * add changelog Signed-off-by: panguixin <[email protected]> * apply review comments Signed-off-by: panguixin <[email protected]> --------- Signed-off-by: panguixin <[email protected]>
1 parent 03b1306 commit ffa67f9

File tree

3 files changed

+52
-3
lines changed

3 files changed

+52
-3
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
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
### Removed
2222

2323
### Fixed
24+
- Fix missing value of FieldSort for unsigned_long ([#14963](https://github.com/opensearch-project/OpenSearch/pull/14963))
2425

2526
### Security
2627

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

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import org.opensearch.action.bulk.BulkRequestBuilder;
4343
import org.opensearch.action.index.IndexRequestBuilder;
4444
import org.opensearch.action.search.SearchPhaseExecutionException;
45+
import org.opensearch.action.search.SearchRequestBuilder;
4546
import org.opensearch.action.search.SearchResponse;
4647
import org.opensearch.action.search.ShardSearchFailure;
4748
import org.opensearch.cluster.metadata.IndexMetadata;
@@ -90,6 +91,7 @@
9091
import static org.opensearch.script.MockScriptPlugin.NAME;
9192
import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING;
9293
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
94+
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertFailures;
9395
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertFirstHit;
9496
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertHitCount;
9597
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertNoFailures;
@@ -919,7 +921,7 @@ public void testSortMissingNumbers() throws Exception {
919921
client().prepareIndex("test")
920922
.setId("3")
921923
.setSource(
922-
jsonBuilder().startObject().field("id", "3").field("i_value", 2).field("d_value", 2.2).field("u_value", 2).endObject()
924+
jsonBuilder().startObject().field("id", "3").field("i_value", 2).field("d_value", 2.2).field("u_value", 3).endObject()
923925
)
924926
.get();
925927

@@ -964,6 +966,18 @@ public void testSortMissingNumbers() throws Exception {
964966
assertThat(searchResponse.getHits().getAt(1).getId(), equalTo("1"));
965967
assertThat(searchResponse.getHits().getAt(2).getId(), equalTo("3"));
966968

969+
logger.info("--> sort with custom missing value");
970+
searchResponse = client().prepareSearch()
971+
.setQuery(matchAllQuery())
972+
.addSort(SortBuilders.fieldSort("i_value").order(SortOrder.ASC).missing(randomBoolean() ? 1 : "1"))
973+
.get();
974+
assertNoFailures(searchResponse);
975+
976+
assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L));
977+
assertThat(searchResponse.getHits().getAt(0).getId(), equalTo("1"));
978+
assertThat(searchResponse.getHits().getAt(1).getId(), equalTo("2"));
979+
assertThat(searchResponse.getHits().getAt(2).getId(), equalTo("3"));
980+
967981
// FLOAT
968982
logger.info("--> sort with no missing (same as missing _last)");
969983
searchResponse = client().prepareSearch()
@@ -1001,6 +1015,18 @@ public void testSortMissingNumbers() throws Exception {
10011015
assertThat(searchResponse.getHits().getAt(1).getId(), equalTo("1"));
10021016
assertThat(searchResponse.getHits().getAt(2).getId(), equalTo("3"));
10031017

1018+
logger.info("--> sort with custom missing value");
1019+
searchResponse = client().prepareSearch()
1020+
.setQuery(matchAllQuery())
1021+
.addSort(SortBuilders.fieldSort("d_value").order(SortOrder.ASC).missing(randomBoolean() ? 1.1 : "1.1"))
1022+
.get();
1023+
assertNoFailures(searchResponse);
1024+
1025+
assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L));
1026+
assertThat(searchResponse.getHits().getAt(0).getId(), equalTo("1"));
1027+
assertThat(searchResponse.getHits().getAt(1).getId(), equalTo("2"));
1028+
assertThat(searchResponse.getHits().getAt(2).getId(), equalTo("3"));
1029+
10041030
// UNSIGNED_LONG
10051031
logger.info("--> sort with no missing (same as missing _last)");
10061032
searchResponse = client().prepareSearch()
@@ -1037,6 +1063,24 @@ public void testSortMissingNumbers() throws Exception {
10371063
assertThat(searchResponse.getHits().getAt(0).getId(), equalTo("2"));
10381064
assertThat(searchResponse.getHits().getAt(1).getId(), equalTo("1"));
10391065
assertThat(searchResponse.getHits().getAt(2).getId(), equalTo("3"));
1066+
1067+
logger.info("--> sort with custom missing value");
1068+
searchResponse = client().prepareSearch()
1069+
.setQuery(matchAllQuery())
1070+
.addSort(SortBuilders.fieldSort("u_value").order(SortOrder.ASC).missing(randomBoolean() ? 2 : "2"))
1071+
.get();
1072+
assertNoFailures(searchResponse);
1073+
1074+
assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L));
1075+
assertThat(searchResponse.getHits().getAt(0).getId(), equalTo("1"));
1076+
assertThat(searchResponse.getHits().getAt(1).getId(), equalTo("2"));
1077+
assertThat(searchResponse.getHits().getAt(2).getId(), equalTo("3"));
1078+
1079+
logger.info("--> sort with negative missing value");
1080+
SearchRequestBuilder searchRequestBuilder = client().prepareSearch()
1081+
.setQuery(matchAllQuery())
1082+
.addSort(SortBuilders.fieldSort("u_value").order(SortOrder.ASC).missing(randomBoolean() ? -1 : "-1"));
1083+
assertFailures(searchRequestBuilder, RestStatus.BAD_REQUEST, containsString("Value [-1] is out of range for an unsigned long"));
10401084
}
10411085

10421086
public void testSortMissingNumbersMinMax() throws Exception {

‎server/src/main/java/org/opensearch/index/fielddata/fieldcomparator/UnsignedLongValuesComparatorSource.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,9 +81,13 @@ public Object missingObject(Object missingValue, boolean reversed) {
8181
return min ? Numbers.MIN_UNSIGNED_LONG_VALUE : Numbers.MAX_UNSIGNED_LONG_VALUE;
8282
} else {
8383
if (missingValue instanceof Number) {
84-
return ((Number) missingValue);
84+
return Numbers.toUnsignedLongExact((Number) missingValue);
8585
} else {
86-
return new BigInteger(missingValue.toString());
86+
BigInteger missing = new BigInteger(missingValue.toString());
87+
if (missing.signum() < 0) {
88+
throw new IllegalArgumentException("Value [" + missingValue + "] is out of range for an unsigned long");
89+
}
90+
return missing;
8791
}
8892
}
8993
}

0 commit comments

Comments
 (0)