-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Reapply "Switch percentiles implementation to MergingDigest (#18124)" #19648
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
Reapply "Switch percentiles implementation to MergingDigest (#18124)" #19648
Conversation
…ch-project#18124)" (opensearch-project#18497) This reverts commit afb08a0. Signed-off-by: Peter Alfonsi <[email protected]>
Signed-off-by: Peter Alfonsi <[email protected]>
Signed-off-by: Peter Alfonsi <[email protected]>
❌ Gradle check result for a21f41d: 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? |
Flaky test: #14294 |
Signed-off-by: Peter Alfonsi <[email protected]>
❌ Gradle check result for 471d6f8: 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? |
Flaky test: #14294 |
Signed-off-by: Peter Alfonsi <[email protected]>
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.
Mostly LGTM, except for couple of comments!
server/src/main/java/org/opensearch/search/aggregations/metrics/TDigestState.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/search/aggregations/metrics/TDigestState.java
Outdated
Show resolved
Hide resolved
❌ Gradle check result for 3a46b83: 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: Peter Alfonsi <[email protected]>
Signed-off-by: Peter Alfonsi <[email protected]>
❌ Gradle check result for 2bb35c8: 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? |
Flaky test: #18947 |
Signed-off-by: Peter Alfonsi <[email protected]>
❌ Gradle check result for da54be2: 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? |
server/src/main/java/org/opensearch/search/aggregations/metrics/TDigestState.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Peter Alfonsi <[email protected]>
❕ Gradle check result for 2c6a6f1: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19648 +/- ##
============================================
+ Coverage 73.09% 73.12% +0.03%
- Complexity 70723 70775 +52
============================================
Files 5725 5725
Lines 323796 323819 +23
Branches 46886 46890 +4
============================================
+ Hits 236673 236795 +122
+ Misses 68009 67954 -55
+ Partials 19114 19070 -44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ch-project#18124)" (opensearch-project#19648) Signed-off-by: Peter Alfonsi <[email protected]> Co-authored-by: Peter Alfonsi <[email protected]>
Description
Un-reverts #18124 which was previously reverted in advance of the 3.1 release in #18497. After discussion with @jainankitk and @msfroh we think it's safe to proceed with the faster MergingDigest implementation.
Also removes the test
TDigestStateTests.testMoreThan4BValues
. It was flaky after switching implementations (see discussion in #18476). But, its entire purpose was to check for regressions on this issue which is specific to the int[]/long[] array inAVLTreeDigest
, so it isn't needed at all if we useMergingDigest
and can be safely deleted.Related Issues
Resolves #18122
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.