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

Introduce execution_hint for Cardinality aggregation #17312

Merged
merged 23 commits into from
Feb 20, 2025

Conversation

asimmahmood1
Copy link
Contributor

@asimmahmood1 asimmahmood1 commented Feb 10, 2025

Description

Introduce execution_hint for Cardinality aggregation - Revive PR #15764

This is intended as short term solution as a workaround to users that see large latency regression when upgrading to OS 2.x. The main risk here is ordinals take up more memory, i.e. depends on size of cardinality. So for very high cardinality, there is a risk of OOM at shard node. This is marked as power user.

I plan to follow up with #15269 which is more useful setting. This will allow the user select a higher threshold for using ordinals, e.g. a shard with large heap can use ordinals more often, but still fall back to the slower direct as needed.

Related Issues

Resolves #16837.

Check List

  • Functionality includes testing.
  • API changes companion pull request [n/a]
  • Public documentation issue/PR created,

Testing

Tested using big5 dataset, see #16837 (comment)

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.

Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Copy link
Contributor

❌ Gradle check result for 0fafb49:

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 0fafb49: 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 e1279da: SUCCESS

Signed-off-by: Asim Mahmood <[email protected]>
Copy link
Contributor

✅ Gradle check result for 8ae7b14: SUCCESS

@asimmahmood1 asimmahmood1 requested a review from msfroh February 18, 2025 17:55
@msfroh msfroh merged commit e3a6cca into opensearch-project:main Feb 20, 2025
33 of 36 checks passed
@asimmahmood1 asimmahmood1 deleted the execution_hint branch February 20, 2025 17:52
@mch2 mch2 added the backport 2.x Backport to 2.x branch label Feb 20, 2025
@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-17312-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e3a6ccadc942c64e83bd224031bc4d1c6ab14623
# Push it to GitHub
git push --set-upstream origin backport/backport-17312-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-17312-to-2.x.

asimmahmood1 added a commit to asimmahmood1/OpenSearch that referenced this pull request Feb 21, 2025
…oject#17312) - backport to 2.19

---------

Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Asim Mahmood <[email protected]>
Signed-off-by: Asim M <[email protected]>
Co-authored-by: Siddharth Rayabharam <[email protected]>
Co-authored-by: Craig Perkins <[email protected]>
(cherry picked from commit e3a6cca)
andrross pushed a commit that referenced this pull request Feb 21, 2025
…kport to 2.19 (#17420)

* Introduce `execution_hint` for Cardinality aggregation (#17312) - backport to 2.19

---------

Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Asim Mahmood <[email protected]>
Signed-off-by: Asim M <[email protected]>
Co-authored-by: Siddharth Rayabharam <[email protected]>
Co-authored-by: Craig Perkins <[email protected]>
(cherry picked from commit e3a6cca)

* Update changelog

Signed-off-by: Asim Mahmood <[email protected]>

---------

Signed-off-by: Asim Mahmood <[email protected]>
cwperks pushed a commit that referenced this pull request Feb 27, 2025
…kport to 2.x (#17419)

* Introduce `execution_hint` for Cardinality aggregation (#17312)

---------

Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Asim Mahmood <[email protected]>
Signed-off-by: Asim M <[email protected]>
Co-authored-by: Siddharth Rayabharam <[email protected]>
Co-authored-by: Craig Perkins <[email protected]>
(cherry picked from commit e3a6cca)

* Update changlog

Signed-off-by: Asim Mahmood <[email protected]>

* Update min version to 2_19_1

* since 2.19 version has been merged:
  #17420

Signed-off-by: Asim Mahmood <[email protected]>

---------

Signed-off-by: Asim Mahmood <[email protected]>
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 good first issue Good for newcomers Search:Aggregations Search:Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Use of Binary DocValue for high cardinality fields to improve aggregations performance
7 participants