GlobalAggregation with profile option enabled returns incorrect result (

#7114) Signed-off-by: Sorabh Hamirwasia <[email protected]> (cherry picked from commit 169e237) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-project · Apr 12, 2023 · 6c03946 · 6c03946
1 parent ef74d54
commit 6c03946
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 3 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -59,6 +59,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fix fuzziness validation ([#5805](https://github.com/opensearch-project/OpenSearch/pull/5805))
 - Avoid negative memory result in IndicesQueryCache stats calculation ([#6917](https://github.com/opensearch-project/OpenSearch/pull/6917))
 - Fix GetSnapshots to not return non-existent snapshots with ignore_unavailable=true ([#6839](https://github.com/opensearch-project/OpenSearch/pull/6839))
+- Fix GlobalAggregation with profile option enabled returns incorrect result ([#7114](https://github.com/opensearch-project/OpenSearch/pull/7114))
 
 ### Security
 

diff --git a/server/src/internalClusterTest/java/org/opensearch/search/aggregations/bucket/GlobalIT.java b/server/src/internalClusterTest/java/org/opensearch/search/aggregations/bucket/GlobalIT.java
@@ -83,10 +83,19 @@ public void setupSuiteScopeCluster() throws Exception {
         ensureSearchable();
     }
 
-    public void testWithStatsSubAggregator() throws Exception {
+    public void testWithStatsSubAggregatorAndProfileEnabled() throws Exception {
+        testWithStatsSubAggregator(true);
+    }
+
+    public void testWithStatsSubAggregatorAndProfileDisabled() throws Exception {
+        testWithStatsSubAggregator(false);
+    }
+
+    private void testWithStatsSubAggregator(boolean profileEnabled) throws Exception {
         SearchResponse response = client().prepareSearch("idx")
             .setQuery(QueryBuilders.termQuery("tag", "tag1"))
             .addAggregation(global("global").subAggregation(stats("value_stats").field("value")))
+            .setProfile(profileEnabled)
             .get();
 
         assertSearchResponse(response);

diff --git a/server/src/main/java/org/opensearch/search/aggregations/AggregationPhase.java b/server/src/main/java/org/opensearch/search/aggregations/AggregationPhase.java
@@ -38,6 +38,7 @@
 import org.opensearch.common.lucene.search.Queries;
 import org.opensearch.search.aggregations.bucket.global.GlobalAggregator;
 import org.opensearch.search.internal.SearchContext;
+import org.opensearch.search.profile.aggregation.ProfilingAggregator;
 import org.opensearch.search.profile.query.CollectorResult;
 import org.opensearch.search.profile.query.InternalProfileCollector;
 import org.opensearch.search.query.QueryPhaseExecutionException;
@@ -67,7 +68,7 @@ public void preProcess(SearchContext context) {
                 AggregatorFactories factories = context.aggregations().factories();
                 aggregators = factories.createTopLevelAggregators(context);
                 for (int i = 0; i < aggregators.length; i++) {
-                    if (aggregators[i] instanceof GlobalAggregator == false) {
+                    if (!isGlobalAggregator(context, aggregators[i])) {
                         collectors.add(aggregators[i]);
                     }
                 }
@@ -106,7 +107,7 @@ public void execute(SearchContext context) {
         Aggregator[] aggregators = context.aggregations().aggregators();
         List<Aggregator> globals = new ArrayList<>();
         for (int i = 0; i < aggregators.length; i++) {
-            if (aggregators[i] instanceof GlobalAggregator) {
+            if (isGlobalAggregator(context, aggregators[i])) {
                 globals.add(aggregators[i]);
             }
         }
@@ -169,4 +170,16 @@ private Collector createCollector(SearchContext context, List<Aggregator> collec
         }
         return collector;
     }
+
+    /**
+     * Checks if passed in aggregator is of type {@link GlobalAggregator}. This method takes care of Aggregator wrapped in
+     * {@link ProfilingAggregator} too
+     * @param context {@link SearchContext}
+     * @param aggregator input {@link Aggregator} instance to evaluate
+     * @return true input is {@link GlobalAggregator} instance or false otherwise
+     */
+    private boolean isGlobalAggregator(SearchContext context, Aggregator aggregator) {
+        return (aggregator instanceof GlobalAggregator
+            || (context.getProfilers() != null && ProfilingAggregator.unwrap(aggregator) instanceof GlobalAggregator));
+    }
 }