From 9d8295599c4ae909a098a6dd34f3485f995299e3 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Thu, 20 Apr 2023 20:42:25 +0200 Subject: [PATCH 1/3] Replace list of collectors with array in profile collector It simplifies things if we can just use an array to hold the children collectors in InternalProfileCollector, and adjust its constructor to take a varargs argument of collectors --- .../search/aggregations/AggregationPhase.java | 4 ++-- .../java/org/elasticsearch/search/dfs/DfsPhase.java | 6 +----- .../profile/query/InternalProfileCollector.java | 8 ++++---- .../search/query/QueryCollectorContext.java | 12 ++++++------ 4 files changed, 13 insertions(+), 17 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationPhase.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationPhase.java index 7e35bf6f90e6d..8ae74fdc712f7 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationPhase.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationPhase.java @@ -52,12 +52,12 @@ public static void preProcess(SearchContext context) { } Collector collector = context.getProfilers() == null ? BucketCollector.NO_OP_COLLECTOR - : new InternalProfileCollector(BucketCollector.NO_OP_COLLECTOR, CollectorResult.REASON_AGGREGATION, List.of()); + : new InternalProfileCollector(BucketCollector.NO_OP_COLLECTOR, CollectorResult.REASON_AGGREGATION); context.registerAggsCollector(collector); } else { Collector collector = context.getProfilers() == null ? bucketCollector.asCollector() - : new InternalProfileCollector(bucketCollector.asCollector(), CollectorResult.REASON_AGGREGATION, List.of()); + : new InternalProfileCollector(bucketCollector.asCollector(), CollectorResult.REASON_AGGREGATION); context.registerAggsCollector(collector); } } diff --git a/server/src/main/java/org/elasticsearch/search/dfs/DfsPhase.java b/server/src/main/java/org/elasticsearch/search/dfs/DfsPhase.java index 0812283cea1d6..aebd874006459 100644 --- a/server/src/main/java/org/elasticsearch/search/dfs/DfsPhase.java +++ b/server/src/main/java/org/elasticsearch/search/dfs/DfsPhase.java @@ -178,11 +178,7 @@ private void executeKnnVectorQuery(SearchContext context) throws IOException { TopScoreDocCollector topScoreDocCollector = TopScoreDocCollector.create(knnSearch.get(i).k(), Integer.MAX_VALUE); Collector collector = topScoreDocCollector; if (context.getProfilers() != null) { - InternalProfileCollector ipc = new InternalProfileCollector( - topScoreDocCollector, - CollectorResult.REASON_SEARCH_TOP_HITS, - List.of() - ); + InternalProfileCollector ipc = new InternalProfileCollector(topScoreDocCollector, CollectorResult.REASON_SEARCH_TOP_HITS); QueryProfiler knnProfiler = context.getProfilers().getDfsProfiler().addQueryProfiler(ipc); collector = ipc; // Set the current searcher profiler to gather query profiling information for gathering top K docs diff --git a/server/src/main/java/org/elasticsearch/search/profile/query/InternalProfileCollector.java b/server/src/main/java/org/elasticsearch/search/profile/query/InternalProfileCollector.java index c744ed073ae70..2ad55b3021d27 100644 --- a/server/src/main/java/org/elasticsearch/search/profile/query/InternalProfileCollector.java +++ b/server/src/main/java/org/elasticsearch/search/profile/query/InternalProfileCollector.java @@ -42,11 +42,11 @@ public class InternalProfileCollector implements Collector { private final ProfileCollector collector; /** - * A list of "embedded" children collectors + * An array of "embedded" children collectors */ - private final List children; + private final InternalProfileCollector[] children; - public InternalProfileCollector(Collector collector, String reason, List children) { + public InternalProfileCollector(Collector collector, String reason, InternalProfileCollector... children) { this.collector = new ProfileCollector(collector); this.reason = reason; this.collectorName = deriveCollectorName(collector); @@ -115,7 +115,7 @@ public CollectorResult getCollectorTree() { } private static CollectorResult doGetCollectorTree(InternalProfileCollector collector) { - List childResults = new ArrayList<>(collector.children.size()); + List childResults = new ArrayList<>(collector.children.length); for (InternalProfileCollector child : collector.children) { CollectorResult result = doGetCollectorTree(child); childResults.add(result); diff --git a/server/src/main/java/org/elasticsearch/search/query/QueryCollectorContext.java b/server/src/main/java/org/elasticsearch/search/query/QueryCollectorContext.java index d53e035ac6dbf..d6d7fb1e30557 100644 --- a/server/src/main/java/org/elasticsearch/search/query/QueryCollectorContext.java +++ b/server/src/main/java/org/elasticsearch/search/query/QueryCollectorContext.java @@ -57,7 +57,10 @@ public ScoreMode scoreMode() { */ protected InternalProfileCollector createWithProfiler(InternalProfileCollector in) throws IOException { final Collector collector = create(in); - return new InternalProfileCollector(collector, profilerName, in != null ? Collections.singletonList(in) : Collections.emptyList()); + if (in == null) { + return new InternalProfileCollector(collector, profilerName); + } + return new InternalProfileCollector(collector, profilerName, in); } /** @@ -132,14 +135,11 @@ Collector create(Collector in) { @Override protected InternalProfileCollector createWithProfiler(InternalProfileCollector in) { - final List subCollectors = new ArrayList<>(); - subCollectors.add(in); if (subCollector instanceof InternalProfileCollector == false) { throw new IllegalArgumentException("non-profiling collector"); } - subCollectors.add((InternalProfileCollector) subCollector); - final Collector collector = MultiCollector.wrap(subCollectors); - return new InternalProfileCollector(collector, REASON_SEARCH_MULTI, subCollectors); + final Collector collector = MultiCollector.wrap(in, subCollector); + return new InternalProfileCollector(collector, REASON_SEARCH_MULTI, in, (InternalProfileCollector) subCollector); } }; } From 28832a40b65339628ed2b6fcb2d8475650aa3ad8 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Thu, 20 Apr 2023 21:33:10 +0200 Subject: [PATCH 2/3] spotless --- .../org/elasticsearch/search/query/QueryCollectorContext.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/query/QueryCollectorContext.java b/server/src/main/java/org/elasticsearch/search/query/QueryCollectorContext.java index d6d7fb1e30557..9ed53fdf1c026 100644 --- a/server/src/main/java/org/elasticsearch/search/query/QueryCollectorContext.java +++ b/server/src/main/java/org/elasticsearch/search/query/QueryCollectorContext.java @@ -21,7 +21,6 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.Collections; import java.util.List; import static org.elasticsearch.search.profile.query.CollectorResult.REASON_SEARCH_MIN_SCORE; From aa999174408b86d81a1a586753e9edb4aff712b3 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Thu, 20 Apr 2023 21:36:34 +0200 Subject: [PATCH 3/3] add null check --- .../search/profile/query/InternalProfileCollector.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/search/profile/query/InternalProfileCollector.java b/server/src/main/java/org/elasticsearch/search/profile/query/InternalProfileCollector.java index 2ad55b3021d27..5eb5c057c1479 100644 --- a/server/src/main/java/org/elasticsearch/search/profile/query/InternalProfileCollector.java +++ b/server/src/main/java/org/elasticsearch/search/profile/query/InternalProfileCollector.java @@ -16,6 +16,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.List; +import java.util.Objects; /** * This class wraps a Lucene Collector and times the execution of: @@ -50,6 +51,10 @@ public InternalProfileCollector(Collector collector, String reason, InternalProf this.collector = new ProfileCollector(collector); this.reason = reason; this.collectorName = deriveCollectorName(collector); + Objects.requireNonNull(children, "children collectors cannot be null"); + for (InternalProfileCollector child : children) { + Objects.requireNonNull(child, "child collector cannot be null"); + } this.children = children; }