Fix multi-value sort for unsigned long

[bugmakerrrrrr]

Description

Fix #16698

Related Issues

Resolves #16698

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[bugmakerrrrrr]

@reta ping :)

[github-actions]

❌ Gradle check result for 82c08dd: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[reta]

@reta ping :)

My apologies @bugmakerrrrrr , I haven't had time to look at the issue yet, I believe your approach is viable but there could be a simpler option to fall back to doubles for unsigned long, I will check it shortly and get back to you. Thanks !

[bugmakerrrrrr]

@reta Thank you for your reply. I actually considered the double method, but I think there are two issues with it. One is that the maximum integer value that double can accurately represent is 2^53, if we use double to handle unsigned long, there may be loss of precision and we get wrong result. The other is that doc values in a document is possible unsorted, so we still need to special handling of min/max/median. Please correct me if I misunderstood something.

[github-actions]

❌ Gradle check result for 29c7e46: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 257ee07: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 257ee07: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[bugmakerrrrrr]

@reta Sorry for disturbing you during the holidays. Thank you for sharing this update. I like the idea, it's more clean and consistent. I'll review the latest change soon. Hope you have a wonderful holiday.

[reta]

@reta Sorry for disturbing you during the holidays.

Thanks @bugmakerrrrrr , no worries at all, just a bit difficult to find the time, happy holidays to you as well!

[rishabh6788]

@bugmakerrrrrr Do you think it is a good idea to run a quick benchmark as well to check if there is any impact on the query performance as well? If yes, check out https://github.com/opensearch-project/OpenSearch/blob/main/PERFORMANCE_BENCHMARKS.md, id_5 will be good candidate.

[github-actions]

❌ Gradle check result for cfbf241: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for bbb1858: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for bbb1858: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[bugmakerrrrrr]

@bugmakerrrrrr Do you think it is a good idea to run a quick benchmark as well to check if there is any impact on the query performance as well?

I don't think so, this is simply a bugfix, not an enhancement :)

[bugmakerrrrrr]

@reta I have an additional concern. Users may specify the numeric_type option for the sort field in their request, or scripts may depend on the ordered values obtained from the field data. It may be necessary for us to sort the values before returning the document values.(just like we did in SortingBinaryDocValues)

[reta]

@reta I have an additional concern. Users may specify the numeric_type option for the sort field in their request, or scripts may depend on the ordered values obtained from the field data. It may be necessary for us to sort the values before returning the document values.(just like we did in SortingBinaryDocValues)

Thanks @bugmakerrrrrr , it would make sense very likely, I think we could make it a separate issue / fix, right? (since it seems to be not related to multivalues).

[bugmakerrrrrr]

@reta I have an additional concern. Users may specify the numeric_type option for the sort field in their request, or scripts may depend on the ordered values obtained from the field data. It may be necessary for us to sort the values before returning the document values.(just like we did in SortingBinaryDocValues)

Thanks @bugmakerrrrrr , it would make sense very likely, I think we could make it a separate issue / fix, right? (since it seems to be not related to multivalues).

@reta Certainly, I will create a new issue to deal with this.

[reta]

❌ Gradle check result for bbb1858: FAILURE

org.opensearch.smoketest.SmokeTestMultiNodeClientYamlTestSuiteIT.test {yaml=search/260_sort_unsigned_long/test sorting against unsigned_long only fields}

@bugmakerrrrrr these failures should be addressed by #16881 (whatever the fix would be), correct? thank you

[bugmakerrrrrr]

❌ Gradle check result for bbb1858: FAILURE

org.opensearch.smoketest.SmokeTestMultiNodeClientYamlTestSuiteIT.test {yaml=search/260_sort_unsigned_long/test sorting against unsigned_long only fields}

@bugmakerrrrrr these failures should be addressed by #16881 (whatever the fix would be), correct? thank you

@reta Yes

[github-actions]

❕ Gradle check result for 9048c7c: UNSTABLE

• TEST FAILURES:

      1 org.opensearch.remotestore.RemoteStoreIT.testCloseIndexWithNoOpSyncAndFlushForSyncTranslog

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.