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.

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

Fix derived field tests for percentile ranks. #15015

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

mch2
Copy link
Member

@mch2 mch2 commented Jul 29, 2024

Description

Fix derived field tests for percentile ranks.

These tests fail to backport to 2.x becuase 2.x uses a different branch of
tdigest that computes percentiles differently. Rather than chase these over time,
change the assertions to check for the length of results returned instead of their values.

Related Issues

Resolves N/A - caught while backporting this pr #15009 to 2.x, will include this commit with the backport after it lands in main.

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.

These tests fail to backport to 2.x becuase 2.x uses a different branch of
tdigest that computes percentiles differently.  Rather than chase these over time,
change the assertions to check for the length of results returned instead of their values.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
@jed326
Copy link
Collaborator

jed326 commented Jul 29, 2024

Should we backport only this to 2.x instead? Or are you saying that the tdigest branch might move forward on main in the future and we shouldn't try to chase that?

@mch2
Copy link
Member Author

mch2 commented Jul 29, 2024

Should we backport only this to 2.x instead? Or are you saying that the tdigest branch might move forward on main in the future and we shouldn't try to chase that?

Yeah, I didn't want this to be an issue down the line given the actual percentiles are not coming from our code.

Copy link
Contributor

❕ Gradle check result for 65f8f20: 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.

Copy link

codecov bot commented Jul 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.09%. Comparing base (e26608b) to head (65f8f20).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #15015      +/-   ##
============================================
- Coverage     71.89%   71.09%   -0.80%     
+ Complexity    62756    62091     -665     
============================================
  Files          5161     5161              
  Lines        294402   294368      -34     
  Branches      42582    42577       -5     
============================================
- Hits         211654   209290    -2364     
- Misses        65324    67597    +2273     
- Partials      17424    17481      +57     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mch2 mch2 merged commit 0cde7ba into opensearch-project:main Jul 30, 2024
54 of 55 checks passed
mch2 added a commit that referenced this pull request Jul 30, 2024
These tests fail to backport to 2.x becuase 2.x uses a different branch of
tdigest that computes percentiles differently.  Rather than chase these over time,
change the assertions to check for the length of results returned instead of their values.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
(cherry picked from commit 0cde7ba)
Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
mch2 added a commit that referenced this pull request Jul 30, 2024
…ields (#15009)

* [Derived Fields] Add aggregation support for derived fields (#14618)

* Add aggregation support for derived fields

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* add unit test for a terms agg with derived fields

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* Fix license header and add changelog entry

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* move matrix_stats tests to aggs-matrix-stats module

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* Move matrix tests back and add dependency to painless module

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* add tests for all aggregations types and support ip_range

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* Add tests for agg script returned from DerivedFieldType

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* remove children aggs test as its not yet supported

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* Add more tests

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

* fix changelog

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

---------

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
(cherry picked from commit e26608b)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Fix derived field tests for percentile ranks. (#15015)

These tests fail to backport to 2.x becuase 2.x uses a different branch of
tdigest that computes percentiles differently.  Rather than chase these over time,
change the assertions to check for the length of results returned instead of their values.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
(cherry picked from commit 0cde7ba)
Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

---------

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Marc Handalian <marc.handalian@gmail.com>
harshavamsi pushed a commit to harshavamsi/OpenSearch that referenced this pull request Aug 20, 2024
These tests fail to backport to 2.x becuase 2.x uses a different branch of
tdigest that computes percentiles differently.  Rather than chase these over time,
change the assertions to check for the length of results returned instead of their values.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
wdongyu pushed a commit to wdongyu/OpenSearch that referenced this pull request Aug 22, 2024
These tests fail to backport to 2.x becuase 2.x uses a different branch of
tdigest that computes percentiles differently.  Rather than chase these over time,
change the assertions to check for the length of results returned instead of their values.

Signed-off-by: Marc Handalian <marc.handalian@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants