intra segment support for terms bucket aggregation by ajleong623 · Pull Request #20829 · opensearch-project/OpenSearch

ajleong623 · 2026-03-10T21:08:18Z

Description

This change supports intra-segment search for terms aggregations. https://docs.opensearch.org/latest/aggregations/bucket/terms/

Related Issues

Related Initial PR: #19704
Related to: #19694

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.

Signed-off-by: Anthony Leong <aj.leong623@gmail.com>

github-actions · 2026-03-10T21:09:37Z

PR Reviewer Guide 🔍

(Review updated until commit `c039425`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes Sub-PR theme: Enable intra-segment support in TermsAggregatorFactory with unit test Relevant files: server/src/main/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java server/src/test/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregatorTests.java Sub-PR theme: Add integration tests and changelog for intra-segment terms aggregation Relevant files: server/src/internalClusterTest/java/org/opensearch/search/aggregations/bucket/terms/StringTermsIT.java CHANGELOG.md
⚡ Recommended focus areas for review Test Coverage The `testConcurrentStringAggregation` test only indexes 5 documents with 1 document per unique value, and uses a fixed index with 2 shards. This may not adequately test intra-segment concurrent behavior since each segment likely contains only one document. Consider adding more documents per value and verifying doc counts > 1 to better exercise the intra-segment code path. public void testConcurrentStringAggregation() throws Exception { createIndex("test_string_terms", Settings.builder().put("index.number_of_shards", 2).put("index.number_of_replicas", 1).build()); try { List<IndexRequestBuilder> builders = new ArrayList<>(5000); for (int i = 0; i < 5; i++) { builders.add(client().prepareIndex("test_string_terms").setSource("value", "val" + (i + 1))); } indexBulkWithSegments(builders, 2); indexRandomForConcurrentSearch("test_string_terms"); SearchResponse response = client().prepareSearch("test_string_terms") .addAggregation( terms("values").executionHint(randomExecutionHint()) .field("value") .collectMode(randomFrom(SubAggCollectionMode.values())) ) .get(); assertSearchResponse(response); Terms values = response.getAggregations().get("values"); assertThat(values, notNullValue()); assertThat(values.getName(), equalTo("values")); assertThat(values.getBuckets().size(), equalTo(5)); for (int i = 0; i < 5; i++) { Terms.Bucket bucket = values.getBucketByKey("val" + (i + 1)); assertThat(bucket, notNullValue()); assertThat(key(bucket), equalTo("val" + (i + 1))); assertThat(bucket.getDocCount(), equalTo(1L)); } } finally { internalCluster().wipeIndices("test_string_terms"); } } Resource Leak The `testConcurrentStringAggregation` test creates an index with `number_of_replicas=1`, but in a test environment there may not be enough nodes to allocate replicas. This could cause the test to hang or fail. Consider using `number_of_replicas=0` or ensuring the cluster has sufficient nodes. createIndex("test_string_terms", Settings.builder().put("index.number_of_shards", 2).put("index.number_of_replicas", 1).build()); Weak Assertion The unit test `testStringTermAggregatorWithIntrasegmentSearch` only asserts that `allFactoriesSupportIntraSegmentSearch()` returns true, but does not actually run an aggregation with intra-segment search enabled and verify correctness of results. The test validates factory configuration but not actual execution behavior. public void testStringTermAggregatorWithIntrasegmentSearch() throws IOException { MappedFieldType fieldtype = new KeywordFieldMapper.KeywordFieldType("value"); try (Directory directory = newDirectory(); RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) { indexWriter.addDocument(singleton(new StringField("value", "1", Field.Store.NO))); try (IndexReader reader = indexWriter.getReader()) { IndexSearcher searcher = newIndexSearcher(reader); AggregatorFactories factories = AggregatorFactories.builder() .addAggregator(new TermsAggregationBuilder("test").field("value")) .build( createSearchContext(searcher, createIndexSettings(), new MatchAllDocsQuery(), null, fieldtype) .getQueryShardContext(), null ); assertTrue(factories.allFactoriesSupportIntraSegmentSearch()); } } }

Signed-off-by: Anthony Leong <aj.leong623@gmail.com>

github-actions · 2026-03-10T21:10:02Z

PR Code Suggestions ✨

Latest suggestions up to c039425

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Fix partition size to exercise concurrent path The `balanced` strategy parameter set uses a `CONCURRENT_SEGMENT_SEARCH_PARTITION_MIN_SEGMENT_SIZE` of 1000, but the test only indexes 5 documents. With a minimum segment size of 1000, the balanced strategy may not actually exercise intra-segment search with such a small dataset, making this test case potentially ineffective. Consider using a smaller value (e.g., 1) to ensure the concurrent path is exercised. server/src/internalClusterTest/java/org/opensearch/search/aggregations/bucket/terms/StringTermsIT.java [103-107] new Object[] { Settings.builder() .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "balanced") - .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_MIN_SEGMENT_SIZE.getKey(), 1000) + .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_MIN_SEGMENT_SIZE.getKey(), 1) .build() } Suggestion importance[1-10]: 6 __ Why: With only 5 documents indexed, a `CONCURRENT_SEGMENT_SEARCH_PARTITION_MIN_SEGMENT_SIZE` of 1000 may prevent the balanced strategy from actually exercising the concurrent/intra-segment search path, making that test parameter potentially ineffective. Using a smaller value like 1 would ensure the concurrent path is actually tested.	Low
General	Fix incorrect initial list capacity The `ArrayList` is initialized with a capacity of 5000, but only 5 elements are added. This is misleading and wastes memory. The initial capacity should match the actual number of elements being added. server/src/internalClusterTest/java/org/opensearch/search/aggregations/bucket/terms/StringTermsIT.java [1096-1099] -List<IndexRequestBuilder> builders = new ArrayList<>(5000); +List<IndexRequestBuilder> builders = new ArrayList<>(5); for (int i = 0; i < 5; i++) { builders.add(client().prepareIndex("test_string_terms").setSource("value", "val" + (i + 1))); } Suggestion importance[1-10]: 4 __ Why: The `ArrayList` is initialized with capacity 5000 but only 5 elements are added, which is misleading. Changing to `new ArrayList<>(5)` is a minor improvement for clarity and memory efficiency, but has negligible practical impact.	Low

Previous suggestions

Suggestions up to commit 789653d

Category	Suggestion	Impact
General	Explicitly enable concurrent search in test parameters The three concurrent segment search parameter sets do not explicitly enable `CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING`, so they rely on the default value being `true`. If the default ever changes, these test cases would silently stop testing concurrent behavior. Consider explicitly setting `CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING` to `true` in each concurrent test parameter to make the intent clear and the tests more robust. server/src/internalClusterTest/java/org/opensearch/search/aggregations/bucket/terms/StringTermsIT.java [98-104] -new Object[] { Settings.builder().put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "segment").build() }, -new Object[] { Settings.builder().put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "force").build() }, +new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true).put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "segment").build() }, +new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true).put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "force").build() }, new Object[] { Settings.builder() + .put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true) .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "balanced") .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_MIN_SEGMENT_SIZE.getKey(), 1000) .build() } Suggestion importance[1-10]: 5 __ Why: The suggestion improves test robustness by making the intent explicit rather than relying on default values for `CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING`. This is a valid defensive testing practice, though it's a minor improvement since the current default is `true`.	Low
General	Verify aggregation results, not just factory support The test only adds a single document and verifies factory-level support, but does not actually execute the aggregation with intra-segment search enabled to verify correctness of results. Consider adding multiple documents with varied values and asserting that the aggregation produces the expected bucket results when intra-segment search is active, to ensure the feature works end-to-end and not just at the factory configuration level. server/src/test/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregatorTests.java [1813-1826] +indexWriter.addDocument(singleton(new StringField("value", "1", Field.Store.NO))); +indexWriter.addDocument(singleton(new StringField("value", "2", Field.Store.NO))); indexWriter.addDocument(singleton(new StringField("value", "1", Field.Store.NO))); try (IndexReader reader = indexWriter.getReader()) { IndexSearcher searcher = newIndexSearcher(reader); AggregatorFactories factories = AggregatorFactories.builder() .addAggregator(new TermsAggregationBuilder("test").field("value")) .build( createSearchContext(searcher, createIndexSettings(), new MatchAllDocsQuery(), null, fieldtype) .getQueryShardContext(), null ); assertTrue(factories.allFactoriesSupportIntraSegmentSearch()); + TermsAggregationBuilder builder = new TermsAggregationBuilder("test").field("value"); + StringTerms result = searchAndReduce(searcher, new MatchAllDocsQuery(), builder, fieldtype); + assertEquals(2, result.getBuckets().size()); + assertEquals(2, result.getBucketByKey("1").getDocCount()); + assertEquals(1, result.getBucketByKey("2").getDocCount()); } Suggestion importance[1-10]: 4 __ Why: The suggestion is valid in that the test only checks factory-level support without verifying actual aggregation results. However, the test is specifically named `testStringTermAggregatorWithIntrasegmentSearch` and its purpose appears to be verifying that the factory reports intra-segment search support, which is a reasonable unit test scope. Adding full end-to-end result verification would be a separate concern and may belong in a different test.	Low

Suggestions up to commit 83527c2

Category	Suggestion	Impact
General	Explicitly enable concurrent search in parameter sets The parameter sets for concurrent segment search strategies do not explicitly enable `CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING`, so these test cases may rely on the default cluster setting being `true`. If the default is `false`, the concurrent strategies would not be exercised. Consider explicitly setting `CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING` to `true` in each concurrent strategy parameter set to ensure the tests actually run with concurrent search enabled. server/src/internalClusterTest/java/org/opensearch/search/aggregations/bucket/terms/StringTermsIT.java [98-104] -new Object[] { Settings.builder().put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "segment").build() }, -new Object[] { Settings.builder().put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "force").build() }, new Object[] { Settings.builder() + .put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true) + .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "segment") + .build() }, +new Object[] { + Settings.builder() + .put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true) + .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "force") + .build() }, +new Object[] { + Settings.builder() + .put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true) .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "balanced") .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_MIN_SEGMENT_SIZE.getKey(), 1000) .build() } Suggestion importance[1-10]: 5 __ Why: This is a valid concern — if `CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING` defaults to `false`, the concurrent strategy parameters would have no effect. Explicitly setting it to `true` ensures the concurrent search path is actually exercised in those test cases.	Low
General	Add result correctness assertions to intra-segment test The test only adds a single document and verifies factory-level support, but does not actually execute the aggregation with intra-segment search enabled to verify correctness of results. Consider adding multiple documents with varied values and asserting the aggregation produces correct bucket counts when intra-segment search is active, to ensure the feature works end-to-end and not just at the factory configuration level. server/src/test/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregatorTests.java [1813-1826] +indexWriter.addDocument(singleton(new StringField("value", "1", Field.Store.NO))); +indexWriter.addDocument(singleton(new StringField("value", "2", Field.Store.NO))); indexWriter.addDocument(singleton(new StringField("value", "1", Field.Store.NO))); try (IndexReader reader = indexWriter.getReader()) { IndexSearcher searcher = newIndexSearcher(reader); AggregatorFactories factories = AggregatorFactories.builder() .addAggregator(new TermsAggregationBuilder("test").field("value")) .build( createSearchContext(searcher, createIndexSettings(), new MatchAllDocsQuery(), null, fieldtype) .getQueryShardContext(), null ); assertTrue(factories.allFactoriesSupportIntraSegmentSearch()); + + TermsAggregationBuilder builder = new TermsAggregationBuilder("test").field("value"); + Terms result = searchAndReduce(searcher, new MatchAllDocsQuery(), builder, fieldtype); + assertEquals(2, result.getBuckets().size()); + assertEquals(2, result.getBucketByKey("1").getDocCount()); + assertEquals(1, result.getBucketByKey("2").getDocCount()); } Suggestion importance[1-10]: 4 __ Why: The suggestion is valid in that the test only checks factory-level support without verifying actual aggregation results. However, the `improved_code` introduces `searchAndReduce` and `Terms` usage that may not be straightforward to integrate in this test context, and the core assertion `assertTrue(factories.allFactoriesSupportIntraSegmentSearch())` is the primary goal of this specific test method.	Low

Suggestions up to commit 5a0993a

Category Suggestion Impact

Possible issue

Enable concurrent search alongside strategy settings

The concurrent segment search parameters are set without also enabling
CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING to true. If the cluster default for
concurrent segment search is false, these strategy settings may have no effect,
making the intra-segment test cases equivalent to the disabled case. Explicitly
enable concurrent segment search alongside the strategy settings.

server/src/internalClusterTest/java/org/opensearch/search/aggregations/bucket/terms/StringTermsIT.java [98-104]

-new Object[] { Settings.builder().put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "segment").build() },
-new Object[] { Settings.builder().put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "force").build() },
+new Object[] { Settings.builder().put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "segment").put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true).build() },
+new Object[] { Settings.builder().put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "force").put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true).build() },
 new Object[] {
     Settings.builder()
         .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "balanced")
         .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_MIN_SEGMENT_SIZE.getKey(), 1000)
+        .put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true)
         .build() }

Suggestion importance[1-10]: 7

__

Why: This is a valid concern - if CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING defaults to false, the partition strategy settings would have no effect, making those test cases redundant with the disabled case. Explicitly enabling concurrent search ensures the intra-segment code paths are actually exercised.

Medium

General

Test should verify aggregation result correctness

The test only adds a single document and verifies factory-level support, but never
actually executes an aggregation to validate correctness of results under
intra-segment search. Consider adding multiple documents across segments and
asserting the aggregation produces correct bucket counts to meaningfully test
intra-segment behavior.

server/src/test/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregatorTests.java [1813-1826]

 indexWriter.addDocument(singleton(new StringField("value", "1", Field.Store.NO)));
+indexWriter.addDocument(singleton(new StringField("value", "2", Field.Store.NO)));
+indexWriter.addDocument(singleton(new StringField("value", "1", Field.Store.NO)));
+indexWriter.forceMerge(2);
 try (IndexReader reader = indexWriter.getReader()) {
     IndexSearcher searcher = newIndexSearcher(reader);
+    TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("test").field("value");
     AggregatorFactories factories = AggregatorFactories.builder()
-        .addAggregator(new TermsAggregationBuilder("test").field("value"))
+        .addAggregator(aggregationBuilder)
         .build(
             createSearchContext(searcher, createIndexSettings(), new MatchAllDocsQuery(), null, fieldtype)
                 .getQueryShardContext(),
             null
         );
     assertTrue(factories.allFactoriesSupportIntraSegmentSearch());
+    StringTerms result = searchAndReduce(searcher, new MatchAllDocsQuery(), aggregationBuilder, fieldtype);
+    assertEquals(2, result.getBuckets().size());
+    assertEquals(2, result.getBucketByKey("1").getDocCount());
+    assertEquals(1, result.getBucketByKey("2").getDocCount());
 }

Suggestion importance[1-10]: 4

__

Why: The suggestion is valid in that the test only checks factory-level support without verifying actual aggregation results. However, the test's primary purpose appears to be verifying allFactoriesSupportIntraSegmentSearch() returns true, which is a focused unit test. Adding full result verification would improve coverage but is a moderate enhancement.

Low

Suggestions up to commit 5b3a8fd

Category	Suggestion	Impact
General	Explicitly enable concurrent search in test parameters The parameter sets for concurrent segment search strategies do not explicitly enable `CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING`, so these test cases may run without concurrent search actually being active, depending on the default setting. Consider explicitly setting `CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING` to `true` in each concurrent strategy parameter set to ensure the intra-segment code paths are exercised. server/src/internalClusterTest/java/org/opensearch/search/aggregations/bucket/terms/StringTermsIT.java [98-104] -new Object[] { Settings.builder().put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "segment").build() }, -new Object[] { Settings.builder().put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "force").build() }, new Object[] { Settings.builder() + .put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true) + .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "segment") + .build() }, +new Object[] { + Settings.builder() + .put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true) + .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "force") + .build() }, +new Object[] { + Settings.builder() + .put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true) .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "balanced") .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_MIN_SEGMENT_SIZE.getKey(), 1000) .build() } Suggestion importance[1-10]: 6 __ Why: This is a valid concern — without explicitly setting `CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING` to `true`, the concurrent strategy parameters may not actually activate concurrent search, potentially leaving the intra-segment code paths untested. Explicitly enabling the setting ensures the intended code paths are exercised.	Low
General	Validate aggregation results, not just factory capability The test only verifies that the factory reports intra-segment search support, but does not actually execute an aggregation to confirm correctness of results under intra-segment search. Consider adding a step that runs the aggregation with an intra-segment search context and asserts the expected bucket results (e.g., one bucket with key "1" and doc count 1). server/src/test/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregatorTests.java [1811-1827] indexWriter.addDocument(singleton(new StringField("value", "1", Field.Store.NO))); try (IndexReader reader = indexWriter.getReader()) { IndexSearcher searcher = newIndexSearcher(reader); + TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("test").field("value"); AggregatorFactories factories = AggregatorFactories.builder() - .addAggregator(new TermsAggregationBuilder("test").field("value")) + .addAggregator(aggregationBuilder) .build( createSearchContext(searcher, createIndexSettings(), new MatchAllDocsQuery(), null, fieldtype) .getQueryShardContext(), null ); assertTrue(factories.allFactoriesSupportIntraSegmentSearch()); + + // Also verify aggregation produces correct results + StringTerms result = searchAndReduce(searcher, new MatchAllDocsQuery(), aggregationBuilder, fieldtype); + assertEquals(1, result.getBuckets().size()); + assertEquals("1", result.getBuckets().get(0).getKeyAsString()); + assertEquals(1L, result.getBuckets().get(0).getDocCount()); } Suggestion importance[1-10]: 4 __ Why: The suggestion to also verify actual aggregation results is valid for improving test coverage, but the test's primary purpose is to verify `allFactoriesSupportIntraSegmentSearch()` returns true for `TermsAggregatorFactory`. The improved code introduces `searchAndReduce` which may not be directly applicable without additional context, and the core functionality being tested (factory capability) is already covered.	Low

Suggestions up to commit d82ee94

Category	Suggestion	Impact
General	Validate aggregation results, not just factory flag The test only verifies that the factory reports intra-segment search support, but never actually executes an aggregation to confirm correctness of results under intra-segment search. Consider adding a step that runs the aggregation and asserts the expected bucket count/key to make the test meaningful. server/src/test/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregatorTests.java [1813-1825] indexWriter.addDocument(singleton(new StringField("value", "1", Field.Store.NO))); try (IndexReader reader = indexWriter.getReader()) { IndexSearcher searcher = newIndexSearcher(reader); + TermsAggregationBuilder builder = new TermsAggregationBuilder("test").field("value"); AggregatorFactories factories = AggregatorFactories.builder() - .addAggregator(new TermsAggregationBuilder("test").field("value")) + .addAggregator(builder) .build( createSearchContext(searcher, createIndexSettings(), new MatchAllDocsQuery(), null, fieldtype) .getQueryShardContext(), null ); assertTrue(factories.allFactoriesSupportIntraSegmentSearch()); + StringTerms result = searchAndReduce(searcher, new MatchAllDocsQuery(), builder, fieldtype); + assertEquals(1, result.getBuckets().size()); + assertEquals("1", result.getBuckets().get(0).getKeyAsString()); } Suggestion importance[1-10]: 5 __ Why: The suggestion is valid - the test only checks that `allFactoriesSupportIntraSegmentSearch()` returns true but doesn't verify actual aggregation correctness under intra-segment search. However, the test's primary purpose appears to be verifying the factory flag, and adding result validation would be a nice-to-have improvement rather than a critical fix.	Low
General	Explicitly enable concurrent search in parameters The three concurrent-search parameter sets do not explicitly enable `CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING`, so they rely on the default value being `true`. If the default ever changes, these cases would silently fall back to non-concurrent execution. Explicitly set the flag to `true` in each concurrent test parameter to make the intent clear and the test robust. server/src/internalClusterTest/java/org/opensearch/search/aggregations/bucket/terms/StringTermsIT.java [98-104] -new Object[] { Settings.builder().put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "segment").build() }, -new Object[] { Settings.builder().put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "force").build() }, +new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true).put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "segment").build() }, +new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true).put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "force").build() }, new Object[] { Settings.builder() + .put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true) .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_STRATEGY.getKey(), "balanced") .put(CONCURRENT_SEGMENT_SEARCH_PARTITION_MIN_SEGMENT_SIZE.getKey(), 1000) .build() } Suggestion importance[1-10]: 4 __ Why: The suggestion is reasonable for making test intent explicit and robust against default value changes. However, it's a defensive coding practice rather than a bug fix, and the current behavior relies on a well-known default that is unlikely to change without broader test failures.	Low

github-actions · 2026-03-10T21:11:18Z

Persistent review updated to latest commit 5b3a8fd

github-actions · 2026-03-10T23:12:59Z

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

Signed-off-by: Anthony Leong <aj.leong623@gmail.com>

github-actions · 2026-03-15T20:35:50Z

Persistent review updated to latest commit 5a0993a

github-actions · 2026-03-15T20:40:06Z

Persistent review updated to latest commit 83527c2

github-actions · 2026-03-15T20:58:52Z

❌ Gradle check result for 83527c2: 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: Anthony Leong <aj.leong623@gmail.com>

github-actions · 2026-03-15T23:11:59Z

Persistent review updated to latest commit 789653d

Signed-off-by: Anthony Leong <aj.leong623@gmail.com>

github-actions · 2026-03-16T00:03:29Z

Persistent review updated to latest commit c039425

github-actions · 2026-03-16T01:17:29Z

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

opensearch-trigger-bot · 2026-04-15T15:41:21Z

This PR is stalled because it has been open for 30 days with no activity.

intrasegment terms agg onboarding

d82ee94

Signed-off-by: Anthony Leong <aj.leong623@gmail.com>

ajleong623 requested review from a team, Bukhtawar, CEHENKLE, Rishikesh1159, anasalkouz, andrross, ashking94, cwperks, dbwiddis, gbbafna, jed326, kotwanikunal, mch2, msfroh, owaiskazi19, reta, sachinpkale, saratvemulapalli, shwetathareja and sohami as code owners March 10, 2026 21:08

ajleong623 marked this pull request as draft March 10, 2026 21:08

changelog

5b3a8fd

Signed-off-by: Anthony Leong <aj.leong623@gmail.com>

spotless

5a0993a

Signed-off-by: Anthony Leong <aj.leong623@gmail.com>

Merge branch 'opensearch-project:main' into intrasegment-terms-agg

83527c2

retry

789653d

Signed-off-by: Anthony Leong <aj.leong623@gmail.com>

add integration test for terms

c039425

Signed-off-by: Anthony Leong <aj.leong623@gmail.com>

opensearch-ci-bot mentioned this pull request Mar 23, 2026

[AUTOCUT] Gradle Check Flaky Test Report for Gradle Test Run :plugins:store-smb:test #20966

Closed

opensearch-trigger-bot Bot added the stalled Issues that have stalled label Apr 15, 2026

Navigation Menu

Uh oh!

intra segment support for terms bucket aggregation - #20829

intra segment support for terms bucket aggregation#20829
ajleong623 wants to merge 6 commits into
opensearch-project:mainfrom
ajleong623:intrasegment-terms-agg

ajleong623 commented Mar 10, 2026

Uh oh!

github-actions Bot commented Mar 10, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Mar 10, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Mar 10, 2026

Uh oh!

github-actions Bot commented Mar 10, 2026

Uh oh!

github-actions Bot commented Mar 15, 2026

Uh oh!

github-actions Bot commented Mar 15, 2026

Uh oh!

github-actions Bot commented Mar 15, 2026

Uh oh!

github-actions Bot commented Mar 15, 2026

Uh oh!

github-actions Bot commented Mar 16, 2026

Uh oh!

github-actions Bot commented Mar 16, 2026

Uh oh!

opensearch-trigger-bot Bot commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

ajleong623 commented Mar 10, 2026

Description

Related Issues

Check List

Uh oh!

github-actions Bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit c039425)

Uh oh!

github-actions Bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented Mar 10, 2026

Uh oh!

github-actions Bot commented Mar 10, 2026

Uh oh!

github-actions Bot commented Mar 15, 2026

Uh oh!

github-actions Bot commented Mar 15, 2026

Uh oh!

github-actions Bot commented Mar 15, 2026

Uh oh!

github-actions Bot commented Mar 15, 2026

Uh oh!

github-actions Bot commented Mar 16, 2026

Uh oh!

github-actions Bot commented Mar 16, 2026

Uh oh!

opensearch-trigger-bot Bot commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

github-actions Bot commented Mar 10, 2026 •

edited

Loading

(Review updated until commit `c039425`)

github-actions Bot commented Mar 10, 2026 •

edited

Loading