Fix multi-value sort for unsigned long #16732
Conversation
bugmakerrrrrr
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami and
VachaShah
as code owners
github-actions
bot
added
bug
|
@reta ping :) |
|
❌ 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? |
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 ! |
|
@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. |
Signed-off-by: panguixin <[email protected]>
|
❌ Gradle check result for 6550f60: 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? |
| @@ -110,6 +111,28 @@ protected void doAssert(Object actualValue, Object expectedValue) { | |||
| } | |||
|
|
|||
| if (expectedValue.equals(actualValue) == false) { | |||
| if (expectedValue instanceof ArrayList && actualValue instanceof ArrayList) { | |||
There was a problem hiding this comment.
You don't need to workaround the comparison, please use index based comparison (instead of array):
- match: { hits.hits.0.sort: [7] }
Would become
- match: { hits.hits.0.sort.0: 7 }
And works out of the box
| @@ -110,6 +111,28 @@ protected void doAssert(Object actualValue, Object expectedValue) { | |||
| } | |||
|
|
|||
| if (expectedValue.equals(actualValue) == false) { | |||
| if (expectedValue instanceof ArrayList && actualValue instanceof ArrayList) { | |||
There was a problem hiding this comment.
You don't need to workaround the comparison, please use index based comparison (instead of array):
- match: { hits.hits.0.sort: [7] }
Would become
- match: { hits.hits.0.sort.0: 7 }
And works out of the box
Signed-off-by: panguixin <[email protected]>
|
❌ Gradle check result for 92843d0: 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? |
|
After executing query phase on data node, the search result will be sent to the coordinator. If the coordinator is on a different node, the result will be serialized before sending, and the sort field will be reduced to the primary type during serialization. But if the coordinator is on the same node, the response will be passed to the handler directly and won't be serialized. In some cases, this may cause different sort field types when merging search results even for one field of one index. When merging top field docs, if the sort field types are not equal, the current implementation uses double to widen sort fields. In this case, the sort values exceeds the MAX_SAFE_INTEGER ( |
Apologies @bugmakerrrrrr , I won't be able to find the time for it during the holidays, just finished with this change, would appreciate your feedback. The |
reta
added
v3.0.0
|
❌ Gradle check result for 49ef878: 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? |
Signed-off-by: Andriy Redko <[email protected]>
|
❌ 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? |
|
❌ 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? |
|
❌ 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? |
|
@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. |
...r/src/main/java/org/opensearch/index/fielddata/SingletonSortedNumericUnsignedLongValues.java
Outdated
Show resolved
Hide resolved
| * @opensearch.internal | ||
| */ | ||
|
|
||
| final class SingletonSortedNumericUnsignedLongValues extends SortedNumericUnsignedLongValues { |
There was a problem hiding this comment.
This doesn't seem to be a singleton values, but simply a wrapper around LongValues, perhaps renamed LongToNumericUnsignedLongValues?
There was a problem hiding this comment.
Yes, it is a wrapper, I was "inspired" by equivalent construct for doubles, but I will think more about your suggestion, perhaps we could simplify things as well, thank you
Thanks @bugmakerrrrrr , no worries at all, just a bit difficult to find the time, happy holidays to you as well! |
|
@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, |
Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: Andriy Redko <[email protected]>
|
❌ 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? |
Description
Fix #16698
Related Issues
Resolves #16698
Check List
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.