-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Attempt to fix the scaled_float precision issue #19188
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.
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
Attempt to fix the scaled_float precision issue #19188
Conversation
Signed-off-by: Prudhvi Godithi <[email protected]>
Signed-off-by: Prudhvi Godithi <[email protected]>
Signed-off-by: Prudhvi Godithi <[email protected]>
|
❌ Gradle check result for e07d545: 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? |
With this change I can see the indexing and query value is same as 9223372036854774784. |
|
❌ Gradle check result for cf3602e: 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: Prudhvi Godithi <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #19188 +/- ##
============================================
+ Coverage 72.79% 72.88% +0.09%
- Complexity 69605 69677 +72
============================================
Files 5658 5658
Lines 320079 320085 +6
Branches 46345 46345
============================================
+ Hits 232996 233290 +294
+ Misses 68230 67899 -331
- Partials 18853 18896 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Prudhvi Godithi <[email protected]>
Signed-off-by: Prudhvi Godithi <[email protected]>
|
❌ Gradle check result for 6655829: 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: Prudhvi Godithi <[email protected]>
|
❌ Gradle check result for 959c5a5: 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? |
modules/mapper-extras/src/main/java/org/opensearch/index/mapper/ScaledFloatFieldMapper.java
Outdated
Show resolved
Hide resolved
|
The PR can solve the term query with the same precision loss. However, precision loss becomes problematic in range query, see #12433 (comment) Alternatively, we can declare this bug in "range": range query currently suffer from inaccurate matching, and it will be |
Yes I was able to reproduce this #12433 (comment), so @kkewwei your suggestion is to continue with existing BigDecimal (the existing behavior) and do not use the direct multiplication ? |
|
I think I got it now, you are planning to introduce a new type |
Yes. |
Signed-off-by: Prudhvi Godithi <[email protected]>
modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldTypeTests.java
Show resolved
Hide resolved
modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldTypeTests.java
Show resolved
Hide resolved
modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldTypeTests.java
Show resolved
Hide resolved
Signed-off-by: Prudhvi Godithi <[email protected]>
|
❌ Gradle check result for d1b5170: 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 d1b5170: 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 d1b5170: 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? |
kkewwei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…9188) * Initial commit Signed-off-by: Prudhvi Godithi <[email protected]> * initial commit Signed-off-by: Prudhvi Godithi <[email protected]> * Fix the precision loss Signed-off-by: Prudhvi Godithi <[email protected]> * Update range query Signed-off-by: Prudhvi Godithi <[email protected]> * Add tests Signed-off-by: Prudhvi Godithi <[email protected]> * UPstream fetch Signed-off-by: Prudhvi Godithi <[email protected]> * Fix tests Signed-off-by: Prudhvi Godithi <[email protected]> * Address comments Signed-off-by: Prudhvi Godithi <[email protected]> * Upstream fetch Signed-off-by: Prudhvi Godithi <[email protected]> --------- Signed-off-by: Prudhvi Godithi <[email protected]>
…9188) * Initial commit Signed-off-by: Prudhvi Godithi <[email protected]> * initial commit Signed-off-by: Prudhvi Godithi <[email protected]> * Fix the precision loss Signed-off-by: Prudhvi Godithi <[email protected]> * Update range query Signed-off-by: Prudhvi Godithi <[email protected]> * Add tests Signed-off-by: Prudhvi Godithi <[email protected]> * UPstream fetch Signed-off-by: Prudhvi Godithi <[email protected]> * Fix tests Signed-off-by: Prudhvi Godithi <[email protected]> * Address comments Signed-off-by: Prudhvi Godithi <[email protected]> * Upstream fetch Signed-off-by: Prudhvi Godithi <[email protected]> --------- Signed-off-by: Prudhvi Godithi <[email protected]>
…9188) * Initial commit Signed-off-by: Prudhvi Godithi <[email protected]> * initial commit Signed-off-by: Prudhvi Godithi <[email protected]> * Fix the precision loss Signed-off-by: Prudhvi Godithi <[email protected]> * Update range query Signed-off-by: Prudhvi Godithi <[email protected]> * Add tests Signed-off-by: Prudhvi Godithi <[email protected]> * UPstream fetch Signed-off-by: Prudhvi Godithi <[email protected]> * Fix tests Signed-off-by: Prudhvi Godithi <[email protected]> * Address comments Signed-off-by: Prudhvi Godithi <[email protected]> * Upstream fetch Signed-off-by: Prudhvi Godithi <[email protected]> --------- Signed-off-by: Prudhvi Godithi <[email protected]> Signed-off-by: Ankit Jain <[email protected]>
…9188) * Initial commit Signed-off-by: Prudhvi Godithi <[email protected]> * initial commit Signed-off-by: Prudhvi Godithi <[email protected]> * Fix the precision loss Signed-off-by: Prudhvi Godithi <[email protected]> * Update range query Signed-off-by: Prudhvi Godithi <[email protected]> * Add tests Signed-off-by: Prudhvi Godithi <[email protected]> * UPstream fetch Signed-off-by: Prudhvi Godithi <[email protected]> * Fix tests Signed-off-by: Prudhvi Godithi <[email protected]> * Address comments Signed-off-by: Prudhvi Godithi <[email protected]> * Upstream fetch Signed-off-by: Prudhvi Godithi <[email protected]> --------- Signed-off-by: Prudhvi Godithi <[email protected]> Signed-off-by: Ankit Jain <[email protected]>
…9188) * Initial commit Signed-off-by: Prudhvi Godithi <[email protected]> * initial commit Signed-off-by: Prudhvi Godithi <[email protected]> * Fix the precision loss Signed-off-by: Prudhvi Godithi <[email protected]> * Update range query Signed-off-by: Prudhvi Godithi <[email protected]> * Add tests Signed-off-by: Prudhvi Godithi <[email protected]> * UPstream fetch Signed-off-by: Prudhvi Godithi <[email protected]> * Fix tests Signed-off-by: Prudhvi Godithi <[email protected]> * Address comments Signed-off-by: Prudhvi Godithi <[email protected]> * Upstream fetch Signed-off-by: Prudhvi Godithi <[email protected]> --------- Signed-off-by: Prudhvi Godithi <[email protected]>
…9188) * Initial commit Signed-off-by: Prudhvi Godithi <[email protected]> * initial commit Signed-off-by: Prudhvi Godithi <[email protected]> * Fix the precision loss Signed-off-by: Prudhvi Godithi <[email protected]> * Update range query Signed-off-by: Prudhvi Godithi <[email protected]> * Add tests Signed-off-by: Prudhvi Godithi <[email protected]> * UPstream fetch Signed-off-by: Prudhvi Godithi <[email protected]> * Fix tests Signed-off-by: Prudhvi Godithi <[email protected]> * Address comments Signed-off-by: Prudhvi Godithi <[email protected]> * Upstream fetch Signed-off-by: Prudhvi Godithi <[email protected]> --------- Signed-off-by: Prudhvi Godithi <[email protected]>
…9188) * Initial commit Signed-off-by: Prudhvi Godithi <[email protected]> * initial commit Signed-off-by: Prudhvi Godithi <[email protected]> * Fix the precision loss Signed-off-by: Prudhvi Godithi <[email protected]> * Update range query Signed-off-by: Prudhvi Godithi <[email protected]> * Add tests Signed-off-by: Prudhvi Godithi <[email protected]> * UPstream fetch Signed-off-by: Prudhvi Godithi <[email protected]> * Fix tests Signed-off-by: Prudhvi Godithi <[email protected]> * Address comments Signed-off-by: Prudhvi Godithi <[email protected]> * Upstream fetch Signed-off-by: Prudhvi Godithi <[email protected]> --------- Signed-off-by: Prudhvi Godithi <[email protected]>
Description
The existing behavior for Indexing uses direct multiplication
(doubleValue * scalingFactor)inparseCreateFieldmethod but Querying usesBigDecimalmultiplication inscale(). I was able to reproduce the issue as shown in #12433 and with this fix I was able to see the correct results. This PR uses same direct multiplication for both Indexing and Querying.Initially I have attempted to use
BigDecimalfor both Indexing and Querying c632a23 which also fixed the issue, but just updating thescale()seemed to be simple as it does not require to reindex the data.Related Issues
Related to #12433
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.