Add a builder for SearchContext #47198

jimczi · 2019-09-27T08:40:22Z

This change removes all subclass of SearchContext and replace them with a simple
Builder than can be used to create final SearchContext that cannot modify
the search request attributes. This commit also ensures that all sub-search
contexts (top_hits agg, inner_hits, percolator, ...) are independent of their
parent context and sets the nested scope in the associated query shard context
if the sub-context is nested.

Relates #46523

This change removes all subclass of SearchContext and replace them with a simple Builder than can be used to create final SearchContext that cannot modify the search request attributes. This commit also ensures that all sub-search contexts (top_hits agg, inner_hits, percolator, ...) are independent of their parent context and sets the nested scope in the associated query shard context if the sub-context is nested. Relates elastic#46523

elasticmachine · 2019-09-27T08:40:24Z

Pinging @elastic/es-search

javanna

I left a couple of small comments/questions, nothing major. Great change!

javanna · 2019-10-02T11:49:25Z

server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java

-        innerHitsContext.version(innerHitBuilder.isVersion());
-        innerHitsContext.seqNoAndPrimaryTerm(innerHitBuilder.isSeqNoAndPrimaryTerm());
-        innerHitsContext.trackScores(innerHitBuilder.isTrackScores());
+    protected SearchContext createSubSearchContext(QueryShardContext cloneShardContext, SearchContext parentContext) throws IOException {


would it be possible to share these different createSubSearchContext methods into a single e.g. SearchContext#createSubSearchContext? Or does each one do and set slightly different things?

I tried a different approach in #47733. We don't need a full search context to run the fetch phase or the highlight phase. I started with the highlight phase but it could be applicable to the fetch phase too and in this case we wouldn't need a sub search context.

javanna · 2019-10-03T09:51:07Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/InnerHitsContext.java


        // TODO: when types are complete removed just use String instead for the id:
        private Uid uid;

-        protected InnerHitSubContext(String name, SearchContext context) {
-            super(context);
+        protected InnerHitsSubContext(String name, SearchContext subSearchContext) {


would it make sense to only accept what's needed here instead of the whole search context? seems like sort, size and searcher may be enough? Though I see that the class member is protected and subclasses need more. Probably good as-is.

javanna · 2019-10-03T12:04:16Z

server/src/main/java/org/elasticsearch/search/internal/SearchContext.java

         */
-        COLLECTION,
+        public Builder buildHighlight(HighlightBuilder builder) throws SearchException {


I see why these methods return the Builder itself, but maybe this should be done only for the set methods? In some cases we also end up never using the return type.

I think it makes more sense to have these as set methods, and then call build on these sub-builders during the parent contexts build method - that is, SearchContextBuilder shouldn't have a SearchContextHighlight member, it should have a HighlightBuilder member.

++, we can require a SearchSourceBuilder and create everything internally. I'll finish the cleanup of the sub search context first and will come back to this afterward.

javanna · 2019-10-03T12:09:34Z

test/framework/src/test/java/org/elasticsearch/test/AbstractQueryTestCaseTests.java

@@ -38,7 +38,7 @@
 /**
 * Various test for {@link org.elasticsearch.test.AbstractQueryTestCase}
 */
-public class AbstractQueryTestCaseTests extends ESTestCase {
+public class  AbstractQueryTestCaseTests extends ESTestCase {


super minor nit: revert this one? ;)

romseygeek

This is going to make reasoning about these things so much easier. I have a few questions, but looks great overall.

romseygeek · 2019-10-07T08:09:51Z

.../percolator/src/main/java/org/elasticsearch/percolator/PercolatorHighlightSubFetchPhase.java

+        SearchContext.Builder builder = new SearchContext.Builder(parentContext.id(),
+            parentContext.getTask(),
+            parentContext.nodeId(),
+            parentContext.indexShard(),
+            parentContext.getQueryShardContext(),
+            parentContext.searcher(),
+            parentContext.fetchPhase(),
+            parentContext.shardTarget().getClusterAlias(),
+            parentContext.numberOfShards(),
+            parentContext::getRelativeTimeInMillis,
+            parentContext.source());


Would this work as a simple copy contructor? Or do we need to be picky about which member variables we're copying over?

I tried a different approach in #47733. We don't need a full search context to run the highlight phase. With this change we wouldn't need to clone the search context at all.

romseygeek · 2019-10-07T08:29:31Z

...er/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregatorFactory.java

+            searchContext, parent, pipelineAggregators, metaData);
+    }
+
+    private SearchContext.Builder parse(SearchContext.Builder builder) {


I don't think this method is needed?

romseygeek · 2019-10-07T08:40:02Z

server/src/main/java/org/elasticsearch/search/internal/SearchContext.java

+     * @param clusterService The cluster service to build the slice query (if any).
+     * @param multiBucketConsumer The bucket consumer for aggregations.
+     */
+    public static Builder parseShardSearchRequest(Builder builder,


Does this make more sense as a method on Builder?

romseygeek · 2019-10-07T08:44:56Z

server/src/main/java/org/elasticsearch/search/internal/SearchContext.java

         */
-        COLLECTION,
+        public Builder buildHighlight(HighlightBuilder builder) throws SearchException {


I think it makes more sense to have these as set methods, and then call build on these sub-builders during the parent contexts build method - that is, SearchContextBuilder shouldn't have a SearchContextHighlight member, it should have a HighlightBuilder member.

romseygeek · 2019-10-07T08:49:51Z

...st/java/org/elasticsearch/search/aggregations/bucket/BestBucketsDeferringCollectorTests.java

@@ -44,8 +44,6 @@
 import java.util.HashSet;
 import java.util.Set;

-import static org.mockito.Mockito.when;


romseygeek · 2019-10-07T08:51:22Z

server/src/test/java/org/elasticsearch/search/aggregations/bucket/GlobalAggregatorTests.java

-        aggregator.postCollection();
-        InternalGlobal result = (InternalGlobal) aggregator.buildAggregation(0L);
-        verify.accept(result, (InternalMin) result.getAggregations().asMap().get("in_global"));
+        try (SearchContext context = aggregator.context()) {


If aggregator.context() is creating a new context that needs to be closed, maybe we should call it buildContext or something similar, to ensure that it doesn't get mistaken for a simple getter?

romseygeek · 2019-10-07T09:16:52Z

server/src/main/java/org/elasticsearch/search/internal/SearchContext.java

+         * Creates the final {@link SearchContext} and set the provided
+         * {@link Runnable} to be executed when the context is closed.
+         */
+        public SearchContext build(Runnable onClose) throws SearchException {


Is there a reason why we take onClose as a parameter here, and don't set it on the Builder directly? Almost every invocation seems to be build(() -> {}) which suggests that it's not necessary.

Today built-in highlighter and plugins have access to the SearchContext through the highlighter context. However most of the information exposed in the SearchContext are not needed and a QueryShardContext would be enough to perform highlighting. This change replaces the SearchContext by the informations that are absolutely required by highlighter: a QueryShardContext and the SearchContextHighlight. This change allows to reduce the exposure of the complex SearchContext and remove the needs to clone it in the percolator sub phase. Relates elastic#47198 Relates elastic#46523

Today built-in highlighter and plugins have access to the SearchContext through the highlighter context. However most of the information exposed in the SearchContext are not needed and a QueryShardContext would be enough to perform highlighting. This change replaces the SearchContext by the informations that are absolutely required by highlighter: a QueryShardContext and the SearchContextHighlight. This change allows to reduce the exposure of the complex SearchContext and remove the needs to clone it in the percolator sub phase. Relates #47198 Relates #46523

jimczi · 2019-10-14T10:52:23Z

I took another approach with #47733 so this pr is obsolete. I'll open a new one that is less invasive.

jimczi added >non-issue :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.5.0 labels Sep 27, 2019

jimczi requested a review from javanna September 27, 2019 08:40

jimczi added 3 commits September 27, 2019 11:08

rewrite query in explain and validate (if requested) APIs

8a00a69

missing change

53d5d3d

fix javadocs

ee553a0

jimczi requested a review from romseygeek October 2, 2019 10:38

Merge branch 'master' into search_context_cleanup

1db961e

javanna reviewed Oct 3, 2019

View reviewed changes

romseygeek suggested changes Oct 7, 2019

View reviewed changes

jimczi mentioned this pull request Oct 8, 2019

Remove the SearchContext from the highlighter context #47733

Merged

jimczi closed this Oct 14, 2019

jimczi deleted the search_context_cleanup branch October 14, 2019 10:52

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a builder for SearchContext #47198

Add a builder for SearchContext #47198

jimczi commented Sep 27, 2019 •

edited

Loading

elasticmachine commented Sep 27, 2019

javanna left a comment

javanna Oct 2, 2019

jimczi Oct 8, 2019

javanna Oct 3, 2019

javanna Oct 3, 2019

romseygeek Oct 7, 2019

jimczi Oct 8, 2019

javanna Oct 3, 2019

romseygeek left a comment

romseygeek Oct 7, 2019

jimczi Oct 8, 2019

romseygeek Oct 7, 2019

romseygeek Oct 7, 2019

romseygeek Oct 7, 2019

romseygeek Oct 7, 2019

romseygeek Oct 7, 2019

romseygeek Oct 7, 2019

jimczi commented Oct 14, 2019

Add a builder for SearchContext #47198

Add a builder for SearchContext #47198

Conversation

jimczi commented Sep 27, 2019 • edited Loading

elasticmachine commented Sep 27, 2019

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romseygeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimczi commented Oct 14, 2019

jimczi commented Sep 27, 2019 •

edited

Loading