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.

Sign up for GitHub

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

Jump to bottom

Fix missing value of FieldSort for unsigned_long #14963

Merged
merged 3 commits into from
Jul 30, 2024
Merged

Fix missing value of FieldSort for unsigned_long #14963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47186f3
Fix missing value of FieldSort for unsigned_long
bugmakerrrrrr Jul 24, 2024
f1bc420
add changelog
bugmakerrrrrr Jul 24, 2024
896e850
apply review comments
bugmakerrrrrr Jul 29, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,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

Expand Down
46 changes: 45 additions & 1 deletion server/src/internalClusterTest/java/org/opensearch/search/sort/FieldSortIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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 {
Expand Down
8 changes: 6 additions & 2 deletions ...va/org/opensearch/index/fielddata/fieldcomparator/UnsignedLongValuesComparatorSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,13 @@
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);

Check warning on line 84 in server/src/main/java/org/opensearch/index/fielddata/fieldcomparator/UnsignedLongValuesComparatorSource.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L84 was not covered by tests
} else {
return new BigInteger(missingValue.toString());
BigInteger missing = new BigInteger(missingValue.toString());

Check warning on line 86 in server/src/main/java/org/opensearch/index/fielddata/fieldcomparator/UnsignedLongValuesComparatorSource.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L86 was not covered by tests
if (missing.signum() < 0) {
throw new IllegalArgumentException("Value [" + missingValue + "] is out of range for an unsigned long");

Check warning on line 88 in server/src/main/java/org/opensearch/index/fielddata/fieldcomparator/UnsignedLongValuesComparatorSource.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L88 was not covered by tests
}
return missing;

Check warning on line 90 in server/src/main/java/org/opensearch/index/fielddata/fieldcomparator/UnsignedLongValuesComparatorSource.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L90 was not covered by tests
}
}
}
Expand Down
Loading