Fix missing value of FieldSort for unsigned_long (#14963) (#15023)

* Fix missing value of FieldSort for unsigned_long * add changelog * apply review comments --------- (cherry picked from commit ffa67f9) Signed-off-by: panguixin <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-project · Jul 30, 2024 · fdd3ea7 · fdd3ea7
1 parent c0e3327
commit fdd3ea7
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 3 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Removed
 
 ### Fixed
+- Fix missing value of FieldSort for unsigned_long ([#14963](https://github.com/opensearch-project/OpenSearch/pull/14963))
 
 ### Security
 

diff --git a/server/src/internalClusterTest/java/org/opensearch/search/sort/FieldSortIT.java b/server/src/internalClusterTest/java/org/opensearch/search/sort/FieldSortIT.java
@@ -42,6 +42,7 @@
 import org.opensearch.action.bulk.BulkRequestBuilder;
 import org.opensearch.action.index.IndexRequestBuilder;
 import org.opensearch.action.search.SearchPhaseExecutionException;
+import org.opensearch.action.search.SearchRequestBuilder;
 import org.opensearch.action.search.SearchResponse;
 import org.opensearch.action.search.ShardSearchFailure;
 import org.opensearch.cluster.metadata.IndexMetadata;
@@ -90,6 +91,7 @@
 import static org.opensearch.script.MockScriptPlugin.NAME;
 import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING;
 import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
+import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertFailures;
 import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertFirstHit;
 import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertHitCount;
 import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertNoFailures;
@@ -919,7 +921,7 @@ public void testSortMissingNumbers() throws Exception {
         client().prepareIndex("test")
             .setId("3")
             .setSource(
-                jsonBuilder().startObject().field("id", "3").field("i_value", 2).field("d_value", 2.2).field("u_value", 2).endObject()
+                jsonBuilder().startObject().field("id", "3").field("i_value", 2).field("d_value", 2.2).field("u_value", 3).endObject()
             )
             .get();
 
@@ -964,6 +966,18 @@ public void testSortMissingNumbers() throws Exception {
         assertThat(searchResponse.getHits().getAt(1).getId(), equalTo("1"));
         assertThat(searchResponse.getHits().getAt(2).getId(), equalTo("3"));
 
+        logger.info("--> sort with custom missing value");
+        searchResponse = client().prepareSearch()
+            .setQuery(matchAllQuery())
+            .addSort(SortBuilders.fieldSort("i_value").order(SortOrder.ASC).missing(randomBoolean() ? 1 : "1"))
+            .get();
+        assertNoFailures(searchResponse);
+
+        assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L));
+        assertThat(searchResponse.getHits().getAt(0).getId(), equalTo("1"));
+        assertThat(searchResponse.getHits().getAt(1).getId(), equalTo("2"));
+        assertThat(searchResponse.getHits().getAt(2).getId(), equalTo("3"));
+
         // FLOAT
         logger.info("--> sort with no missing (same as missing _last)");
         searchResponse = client().prepareSearch()
@@ -1001,6 +1015,18 @@ public void testSortMissingNumbers() throws Exception {
         assertThat(searchResponse.getHits().getAt(1).getId(), equalTo("1"));
         assertThat(searchResponse.getHits().getAt(2).getId(), equalTo("3"));
 
+        logger.info("--> sort with custom missing value");
+        searchResponse = client().prepareSearch()
+            .setQuery(matchAllQuery())
+            .addSort(SortBuilders.fieldSort("d_value").order(SortOrder.ASC).missing(randomBoolean() ? 1.1 : "1.1"))
+            .get();
+        assertNoFailures(searchResponse);
+
+        assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L));
+        assertThat(searchResponse.getHits().getAt(0).getId(), equalTo("1"));
+        assertThat(searchResponse.getHits().getAt(1).getId(), equalTo("2"));
+        assertThat(searchResponse.getHits().getAt(2).getId(), equalTo("3"));
+
         // UNSIGNED_LONG
         logger.info("--> sort with no missing (same as missing _last)");
         searchResponse = client().prepareSearch()
@@ -1037,6 +1063,24 @@ public void testSortMissingNumbers() throws Exception {
         assertThat(searchResponse.getHits().getAt(0).getId(), equalTo("2"));
         assertThat(searchResponse.getHits().getAt(1).getId(), equalTo("1"));
         assertThat(searchResponse.getHits().getAt(2).getId(), equalTo("3"));
+
+        logger.info("--> sort with custom missing value");
+        searchResponse = client().prepareSearch()
+            .setQuery(matchAllQuery())
+            .addSort(SortBuilders.fieldSort("u_value").order(SortOrder.ASC).missing(randomBoolean() ? 2 : "2"))
+            .get();
+        assertNoFailures(searchResponse);
+
+        assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L));
+        assertThat(searchResponse.getHits().getAt(0).getId(), equalTo("1"));
+        assertThat(searchResponse.getHits().getAt(1).getId(), equalTo("2"));
+        assertThat(searchResponse.getHits().getAt(2).getId(), equalTo("3"));
+
+        logger.info("--> sort with negative missing value");
+        SearchRequestBuilder searchRequestBuilder = client().prepareSearch()
+            .setQuery(matchAllQuery())
+            .addSort(SortBuilders.fieldSort("u_value").order(SortOrder.ASC).missing(randomBoolean() ? -1 : "-1"));
+        assertFailures(searchRequestBuilder, RestStatus.BAD_REQUEST, containsString("Value [-1] is out of range for an unsigned long"));
     }
 
     public void testSortMissingNumbersMinMax() throws Exception {

diff --git a/...va/org/opensearch/index/fielddata/fieldcomparator/UnsignedLongValuesComparatorSource.java b/...va/org/opensearch/index/fielddata/fieldcomparator/UnsignedLongValuesComparatorSource.java
@@ -81,9 +81,13 @@ public Object missingObject(Object missingValue, boolean reversed) {
             return min ? Numbers.MIN_UNSIGNED_LONG_VALUE : Numbers.MAX_UNSIGNED_LONG_VALUE;
         } else {
             if (missingValue instanceof Number) {
-                return ((Number) missingValue);
+                return Numbers.toUnsignedLongExact((Number) missingValue);
             } else {
-                return new BigInteger(missingValue.toString());
+                BigInteger missing = new BigInteger(missingValue.toString());
+                if (missing.signum() < 0) {
+                    throw new IllegalArgumentException("Value [" + missingValue + "] is out of range for an unsigned long");
+                }
+                return missing;
             }
         }
     }