Run terms concurrently when cardinality is only lower than shard size (…

…elastic#110369) To make sure the results are the same between concurrent and non-concurrent runs, we need to exclude situations where shard size == cardinality.
dadoonet · Jul 2, 2024 · 2441340 · 2441340
1 parent e0d71d6
commit 2441340
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 7 deletions.
diff --git a/docs/changelog/110369.yaml b/docs/changelog/110369.yaml
@@ -0,0 +1,6 @@
+pr: 110369
+summary: Run terms concurrently when cardinality is only lower than shard size
+area: Aggregations
+type: bug
+issues:
+ - 105505
diff --git a/...main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java b/...main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java
@@ -172,7 +172,8 @@ public static boolean supportsParallelCollection(long cardinality, BucketOrder o
                 return cardinality <= KEY_ORDER_CONCURRENCY_THRESHOLD;
             }
             BucketCountThresholds adjusted = TermsAggregatorFactory.adjustBucketCountThresholds(bucketCountThresholds, order);
-            return cardinality <= adjusted.getShardSize();
+            // for cardinality equal to shard size, we don't know if there were more terms when merging.
+            return cardinality < adjusted.getShardSize();
         }
         return false;
     }

diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/TermsTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/TermsTests.java
@@ -211,8 +211,8 @@ public boolean supportsParallelCollection(ToLongFunction<String> fieldCardinalit
         {
             TermsAggregationBuilder terms = new TermsAggregationBuilder("terms");
             terms.shardSize(10);
-            assertTrue(terms.supportsParallelCollection(field -> randomIntBetween(1, 10)));
-            assertFalse(terms.supportsParallelCollection(field -> randomIntBetween(11, 100)));
+            assertTrue(terms.supportsParallelCollection(field -> randomIntBetween(1, 9)));
+            assertFalse(terms.supportsParallelCollection(field -> randomIntBetween(10, 100)));
         }
         {
             TermsAggregationBuilder terms = new TermsAggregationBuilder("terms");

diff --git a/.../java/org/elasticsearch/xpack/analytics/multiterms/MultiTermsAggregationBuilderTests.java b/.../java/org/elasticsearch/xpack/analytics/multiterms/MultiTermsAggregationBuilderTests.java
@@ -161,10 +161,10 @@ public void testSupportsParallelCollection() {
             List<String> fields = new ArrayList<>();
             assertTrue(builder.supportsParallelCollection(field -> {
                 fields.add(field);
-                return randomIntBetween(0, 10);
+                return randomIntBetween(0, 9);
             }));
             assertEquals(List.of("field1", "field2"), fields);
-            assertFalse(builder.supportsParallelCollection(field -> randomIntBetween(11, 100)));
+            assertFalse(builder.supportsParallelCollection(field -> randomIntBetween(10, 100)));
             terms.terms(
                 List.of(
                     sourceBuilder1.build(),
@@ -183,14 +183,14 @@ public void testSupportsParallelCollection() {
                 List.of(sourceBuilder1.build(), sourceBuilder2.build())
             );
             terms.shardSize(10);
-            assertTrue(terms.supportsParallelCollection(field -> randomIntBetween(0, 10)));
+            assertTrue(terms.supportsParallelCollection(field -> randomIntBetween(0, 9)));
             terms.subAggregation(new TermsAggregationBuilder("name") {
                 @Override
                 public boolean supportsParallelCollection(ToLongFunction<String> fieldCardinalityResolver) {
                     return false;
                 }
             });
-            assertFalse(terms.supportsParallelCollection(field -> randomIntBetween(0, 10)));
+            assertFalse(terms.supportsParallelCollection(field -> randomIntBetween(0, 9)));
         }
         {
             MultiValuesSourceFieldConfig.Builder sourceBuilder1 = new MultiValuesSourceFieldConfig.Builder();