Fix the regression of terms agg optimization

[bowenlan-amzn]

Description

We discovered on clickbench that when terms aggregation is applied on very high cardinality field, the performance actually regressed.

The optimization in terms aggregation uses terms dictionary but also reads the actual BytesRef/value. So for very high cardinality field, this value reading becomes a problem.

This PR introduces a cluster setting to guard the max cardinality supported for terms aggregation. The default value is set to 30,000 for now because we know this optimization worked well for big5 terms aggregation on a field with 26,xxx cardinality.

The fallback logic of streaming aggregation is also updated accordingly

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR introduces a configurable maximum cardinality threshold for terms aggregation precomputation. A new cluster setting controls when terms aggregations should avoid expensive precomputation for high-cardinality fields, integrating the threshold check into aggregation factories and segment processing logic.

Changes

Cohort / File(s)	Summary
Configuration & Settings server/src/main/java/org/opensearch/search/SearchService.java, server/src/main/java/org/opensearch/common/settings/ClusterSettings.java	Added new Setting TERMS_AGGREGATION_MAX_PRECOMPUTE_CARDINALITY with default 30000L and registered it as a built-in cluster setting.
Search Context API server/src/main/java/org/opensearch/search/internal/SearchContext.java, server/src/main/java/org/opensearch/search/DefaultSearchContext.java	Added public API method termsAggregationMaxPrecomputeCardinality() to SearchContext interface and implemented it in DefaultSearchContext with setting evaluation and initialization.
Aggregation Logic server/src/main/java/org/opensearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java, server/src/main/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java	Integrated cardinality threshold checks: added precondition in segment processing to skip high-cardinality fields and tightened streaming decision logic to require cardinality compliance for match-all queries.
Testing server/src/test/java/org/opensearch/search/aggregations/FactoryStreamingCostEstimationTests.java, test/framework/src/main/java/org/opensearch/search/aggregations/AggregatorTestCase.java	Added test for high-cardinality terms aggregation with match-all queries and stubbed termsAggregationMaxPrecomputeCardinality mock return value in test framework.
Documentation CHANGELOG.md	Added changelog entry documenting the regression fix for terms aggregation optimization at high cardinality.

Sequence Diagram(s)

sequenceDiagram
    participant ClusterConfig as Cluster Configuration
    participant SearchCtx as SearchContext
    participant AggFactory as TermsAggregatorFactory
    participant Aggregator as Aggregator Execution
    
    ClusterConfig->>SearchCtx: Initialize with TERMS_AGGREGATION_MAX_PRECOMPUTE_CARDINALITY setting
    SearchCtx->>SearchCtx: termsAggregationMaxPrecomputeCardinality() evaluates and caches threshold
    AggFactory->>SearchCtx: Query threshold for match-all with high cardinality
    SearchCtx-->>AggFactory: Return max precompute cardinality (30000)
    AggFactory->>AggFactory: Check if maxCardinality <= threshold
    alt Cardinality within threshold
        AggFactory->>Aggregator: Proceed with optimized streaming disabled
    else Cardinality exceeds threshold
        AggFactory->>Aggregator: Use streaming for high-cardinality field
    end
    Aggregator->>Aggregator: tryCollectFromTermFrequencies checks segment cardinality
    Aggregator->>Aggregator: Skip precomputation if cardinality exceeds threshold

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• Streaming agg planning fallback enhancement #20510: Both PRs modify streaming decision logic in TermsAggregatorFactory for adjusting when streaming is disabled for terms aggregations in match-all scenarios.

Suggested labels

Search:Aggregations, Search:Performance, v3.5.0

🚥 Pre-merge checks | ✅ 2 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is incomplete; it contains only the template with empty sections and unchecked boxes, providing no actual details about what the change achieves or which issues it resolves.	Fill in the Description section explaining the regression and how it's fixed, provide the related issue number, and confirm testing status in the checklist.
Docstring Coverage	⚠️ Warning	Docstring coverage is 26.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: fixing a regression in terms aggregation optimization, which is directly reflected in the file modifications.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into main

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

✅ Gradle check result for a9b0b3a: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.23%. Comparing base (3aed19d) to head (830fefe).
⚠️ Report is 29 commits behind head on main.

Files with missing lines	Patch %	Lines
...ket/terms/GlobalOrdinalsStringTermsAggregator.java	66.66%	0 Missing and 1 partial ⚠️
.../org/opensearch/search/internal/SearchContext.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20623      +/-   ##
============================================
- Coverage     73.30%   73.23%   -0.07%     
+ Complexity    71965    71934      -31     
============================================
  Files          5781     5781              
  Lines        329200   329210      +10     
  Branches      47491    47492       +1     
============================================
- Hits         241314   241091     -223     
- Misses        68491    68760     +269     
+ Partials      19395    19359      -36     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

✅ Gradle check result for 830fefe: SUCCESS