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

Support Dynamic Pruning in Cardinality Aggregation #13821

Merged
merged 30 commits into from
Jun 11, 2024

Conversation

bowenlan-amzn
Copy link
Member

@bowenlan-amzn bowenlan-amzn commented May 24, 2024

Description

First I want to thanks Rishabh's work on this which I learnt from.

Background

Cardinality aggregation count the number of distinct values of a field.
Some common use cases are counting the visitors of a service, detect anomly of unique IP address or event types and count unique product or brands in e-commerce, etc.

Idea

By defaults, aggregation works on every document that matches the top level query, collects them in an iterative manner.
However, once we collect one document, we don't need to care about the values of this document anymore, since they are already counted as the distinct values in cardinality aggregation results.
So the idea is to stop working on other documents that also have these collected values as the aggregation going on.

Implementation

  1. Segment level

To implement this idea, we need to know all the possible values of the field and the documents of each value before the aggregation started.
Lucene provide a dictionary of terms and a list the documents containing these terms.
We can build a disjunction of the documentID iterators for these term values. Every time after we collect values of a doc, we remove the iterators of the collected. So we actually skipped many documents for the next doc iteration and won't see these collected values anymore.

  1. For numeric field

Lucene provide a dictionary of terms and the documents that contain these terms.
However, for numeric field, lucene provides a tree structure that let user traverse the numeric values in sorted order. So it would be different to do the same kind of dynamic pruning for numeric field. It's still possible but we can work on it as a follow up considering numeric cardinality is less common comparing to keyword term cardinality.

  1. Across segments or Shard level

One shard may contain multiple segments, and by default segments are processed in sequential. If the seen values are saved at shard level, for the subsequential segments, there's no need to build the iterator for these values.
This will be a follow up tasks.

Public Facing Changes

New cluster setting that can dynamically tune the threshold of supported maximum cardinality for doing the pruning
search.dynamic_pruning.cardinality_aggregation_threshold
Set to 0 can disable the feature.
Currently use 100 which should be a safe number.

opensearch-project/documentation-website#7334
opensearch-project/documentation-website#7341

Benchmark

Add cardinality aggregation operations in big5 workload
opensearch-project/opensearch-benchmark-workloads#311

Performance improvements before and after

|                                       50th percentile latency |  cardinality-agg-low |      903463 |     10.3347 |      -903453 |     ms |
|                                       90th percentile latency |  cardinality-agg-low | 1.04617e+06 |     10.8622 | -1.04615e+06 |     ms |
|                                       99th percentile latency |  cardinality-agg-low | 1.07827e+06 |      13.704 | -1.07826e+06 |     ms |
|                                      100th percentile latency |  cardinality-agg-low | 1.08184e+06 |      14.249 | -1.08183e+06 |     ms |
|                                  50th percentile service time |  cardinality-agg-low |     4103.42 |     9.22686 |      -4094.2 |     ms |
|                                  90th percentile service time |  cardinality-agg-low |     4109.98 |     9.54821 |     -4100.43 |     ms |
|                                  99th percentile service time |  cardinality-agg-low |     4112.87 |     13.4495 |     -4099.42 |     ms |
|                                 100th percentile service time |  cardinality-agg-low |     4115.37 |     13.6122 |     -4101.76 |     ms |
|                                                    error rate |  cardinality-agg-low |           0 |           0 |            0 |      % |

|                                       50th percentile latency | cardinality-agg-high |      918502 |      690842 |      -227661 |     ms |
|                                       90th percentile latency | cardinality-agg-high | 1.06362e+06 |      800014 |      -263604 |     ms |
|                                       99th percentile latency | cardinality-agg-high | 1.09628e+06 |      824581 |      -271698 |     ms |
|                                      100th percentile latency | cardinality-agg-high |  1.0999e+06 |      827314 |      -272586 |     ms |
|                                  50th percentile service time | cardinality-agg-high |     4162.98 |     3256.42 |     -906.562 |     ms |
|                                  90th percentile service time | cardinality-agg-high |     4169.53 |     3260.06 |     -909.463 |     ms |
|                                  99th percentile service time | cardinality-agg-high |     4177.45 |     3266.89 |     -910.562 |     ms |
|                                 100th percentile service time | cardinality-agg-high |     4178.73 |     3280.35 |     -898.381 |     ms |
|                                                    error rate | cardinality-agg-high |           0 |           0 |            0 |      % |

Related Issues

Resolves #11959

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • API changes companion pull request created.
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

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.

@github-actions github-actions bot added enhancement Enhancement or improvement to existing feature or request Search:Aggregations v2.15.0 Issues and PRs related to version 2.15.0 labels May 24, 2024
rishabhmaurya and others added 3 commits May 24, 2024 11:02
Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>
Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>
Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>
Copy link
Contributor

❌ Gradle check result for c2775ac: 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?

Copy link
Contributor

❌ Gradle check result for 91f7a04: 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?

Copy link
Contributor

❌ Gradle check result for 8fc9bee: 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: bowenlan-amzn <bowenlan23@gmail.com>
Copy link
Contributor

❌ Gradle check result for 85133c4: 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: bowenlan-amzn <bowenlan23@gmail.com>
@bowenlan-amzn bowenlan-amzn changed the title Cardinality Aggregation Dynamic Pruning Support Dynamic Pruning in Cardinality Aggregation May 24, 2024
Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>
@bowenlan-amzn bowenlan-amzn added the backport 2.x Backport to 2.x branch label May 24, 2024
Copy link
Contributor

❕ Gradle check result for 9d4701c: 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 May 24, 2024

Codecov Report

Attention: Patch coverage is 78.18182% with 24 lines in your changes missing coverage. Please review.

Project coverage is 71.66%. Comparing base (b15cb0c) to head (5f7dad3).
Report is 405 commits behind head on main.

Files Patch % Lines
...ch/aggregations/metrics/CardinalityAggregator.java 78.64% 14 Missing and 8 partials ⚠️
...va/org/opensearch/search/DefaultSearchContext.java 80.00% 1 Missing ⚠️
.../org/opensearch/search/internal/SearchContext.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #13821      +/-   ##
============================================
+ Coverage     71.42%   71.66%   +0.24%     
- Complexity    59978    61795    +1817     
============================================
  Files          4985     5105     +120     
  Lines        282275   290175    +7900     
  Branches      40946    41917     +971     
============================================
+ Hits         201603   207968    +6365     
- Misses        63999    65043    +1044     
- Partials      16673    17164     +491     

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

Copy link
Contributor

❌ Gradle check result for d53182b: 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?

Copy link
Contributor

❌ Gradle check result for 77fceea: 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?

@bowenlan-amzn
Copy link
Member Author

❌ Gradle check result for 77fceea: 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?

Pretty interesting, seems this failure can be reproduced consistently with the seed

./gradlew ':server:internalClusterTest' --tests "org.opensearch.indices.IndicesRequestCacheIT" -Dtests.method="testDeleteAndCreateSameIndexShardOnSameNode {p0={"search.concurrent_segment_search.enabled":"false"}}" -Dtests.seed=8199B3619B8101C

Copy link
Contributor

github-actions bot commented Jun 2, 2024

❌ Gradle check result for 09f957c: 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?

Copy link
Contributor

github-actions bot commented Jun 3, 2024

❌ Gradle check result for 09f957c: null

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?

Copy link
Contributor

✅ Gradle check result for 5f7dad3: SUCCESS

@mch2 mch2 merged commit 2f8cb07 into opensearch-project:main Jun 11, 2024
31 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-13821-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 2f8cb079c2c220304b8e6965853a15732c3bf57c
# Push it to GitHub
git push --set-upstream origin backport/backport-13821-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-13821-to-2.x.

bowenlan-amzn added a commit to bowenlan-amzn/OpenSearch that referenced this pull request Jun 11, 2024
…t#13821)

* Cardinality aggregation dynamic pruning changes

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Reading

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* remaining disjunction scorer full understand

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* utilize competitive iterator api to perform pruning

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* handle missing input

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* add change log

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* clean up

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Clean up

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Test fix

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Do all the scoring within Cardinality

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* clean unnecessary

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* fix

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Add dynamic flag for this feature

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Add random test, small bug fix

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* address comment

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Address comments

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* address comments

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

---------

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>
Co-authored-by: Rishabh Maurya <rishabhmaurya05@gmail.com>
mch2 pushed a commit that referenced this pull request Jun 12, 2024
* Cardinality aggregation dynamic pruning changes



* Reading



* remaining disjunction scorer full understand



* utilize competitive iterator api to perform pruning



* handle missing input



* add change log



* clean up



* Clean up



* Test fix



* Do all the scoring within Cardinality



* clean unnecessary



* fix



* Add dynamic flag for this feature



* Add random test, small bug fix



* address comment



* Address comments



* address comments



---------

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>
Co-authored-by: Rishabh Maurya <rishabhmaurya05@gmail.com>
@bowenlan-amzn bowenlan-amzn deleted the 11959-cardi-agg-pruning branch June 12, 2024 00:52
harshavamsi pushed a commit to harshavamsi/OpenSearch that referenced this pull request Jul 12, 2024
…t#13821)

* Cardinality aggregation dynamic pruning changes

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Reading

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* remaining disjunction scorer full understand

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* utilize competitive iterator api to perform pruning

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* handle missing input

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* add change log

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* clean up

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Clean up

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Test fix

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Do all the scoring within Cardinality

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* clean unnecessary

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* fix

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Add dynamic flag for this feature

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Add random test, small bug fix

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* address comment

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Address comments

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* address comments

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

---------

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>
Co-authored-by: Rishabh Maurya <rishabhmaurya05@gmail.com>
kkewwei pushed a commit to kkewwei/OpenSearch that referenced this pull request Jul 24, 2024
…t#13821) (opensearch-project#14203)

* Cardinality aggregation dynamic pruning changes

* Reading

* remaining disjunction scorer full understand

* utilize competitive iterator api to perform pruning

* handle missing input

* add change log

* clean up

* Clean up

* Test fix

* Do all the scoring within Cardinality

* clean unnecessary

* fix

* Add dynamic flag for this feature

* Add random test, small bug fix

* address comment

* Address comments

* address comments

---------

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>
Co-authored-by: Rishabh Maurya <rishabhmaurya05@gmail.com>
Signed-off-by: kkewwei <kkewwei@163.com>
wdongyu pushed a commit to wdongyu/OpenSearch that referenced this pull request Aug 22, 2024
…t#13821)

* Cardinality aggregation dynamic pruning changes

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Reading

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* remaining disjunction scorer full understand

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* utilize competitive iterator api to perform pruning

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* handle missing input

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* add change log

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* clean up

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Clean up

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Test fix

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Do all the scoring within Cardinality

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* clean unnecessary

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* fix

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Add dynamic flag for this feature

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Add random test, small bug fix

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* address comment

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* Address comments

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

* address comments

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>

---------

Signed-off-by: bowenlan-amzn <bowenlan23@gmail.com>
Co-authored-by: Rishabh Maurya <rishabhmaurya05@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch backport-failed enhancement Enhancement or improvement to existing feature or request Search:Aggregations v2.15.0 Issues and PRs related to version 2.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Make use of dynamic pruning for faster cardinality aggregations
4 participants