From 3f793b625b6851b89816fff2735cbfd899e2592b Mon Sep 17 00:00:00 2001 From: Rishabh Maurya Date: Thu, 6 Feb 2025 15:57:54 -0800 Subject: [PATCH] Prevent unnecessary heap sort when buckets needs to be ordered by key in NumericTermsAggregation (#17252) Signed-off-by: Rishabh Maurya --- CHANGELOG.md | 1 + .../bucket/terms/NumericTermsAggregator.java | 17 ++++++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20c5934049405..66f73880a0ab1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Propagate the sourceIncludes and excludes fields from fetchSourceContext to FieldsVisitor. ([#17080](https://github.com/opensearch-project/OpenSearch/pull/17080)) - [Star Tree] [Search] Resolving Date histogram with metric aggregation using star-tree ([#16674](https://github.com/opensearch-project/OpenSearch/pull/16674)) - [Star Tree] [Search] Extensible design to support different query and field types ([#17137](https://github.com/opensearch-project/OpenSearch/pull/17137)) +- Improve performace of NumericTermAggregation by avoiding unnecessary sorting([#17252](https://github.com/opensearch-project/OpenSearch/pull/17252)) ### Dependencies - Bump `com.google.cloud:google-cloud-core-http` from 2.23.0 to 2.47.0 ([#16504](https://github.com/opensearch-project/OpenSearch/pull/16504)) diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/terms/NumericTermsAggregator.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/terms/NumericTermsAggregator.java index 9d095bbf7dccf..1d78a59a563f0 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/terms/NumericTermsAggregator.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/terms/NumericTermsAggregator.java @@ -64,6 +64,7 @@ import java.io.IOException; import java.math.BigInteger; import java.util.Arrays; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.function.BiConsumer; @@ -202,9 +203,19 @@ private InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws // Get the top buckets B[] bucketsForOrd = buildBuckets(ordered.size()); topBucketsPerOrd[ordIdx] = bucketsForOrd; - for (int b = ordered.size() - 1; b >= 0; --b) { - topBucketsPerOrd[ordIdx][b] = ordered.pop(); - otherDocCounts[ordIdx] -= topBucketsPerOrd[ordIdx][b].getDocCount(); + if (isKeyOrder(order)) { + for (int b = ordered.size() - 1; b >= 0; --b) { + topBucketsPerOrd[ordIdx][b] = ordered.pop(); + otherDocCounts[ordIdx] -= topBucketsPerOrd[ordIdx][b].getDocCount(); + } + } else { + // sorted buckets not needed as they will be sorted by key in buildResult() which is different from + // order in priority queue ordered + Iterator itr = ordered.iterator(); + for (int b = ordered.size() - 1; b >= 0; --b) { + topBucketsPerOrd[ordIdx][b] = itr.next(); + otherDocCounts[ordIdx] -= topBucketsPerOrd[ordIdx][b].getDocCount(); + } } }