From f9530dfe8f360183b42c32828c62743017fc0e47 Mon Sep 17 00:00:00 2001 From: javanna Date: Thu, 8 Sep 2016 00:01:43 +0200 Subject: [PATCH] remove FetchSubPhaseContext in favour of generic fetch sub phase builder of type object The context was an object where the parsed info are stored. That is more of what we call the builder since after the search refactoring. No need for generics in FetchSubPhaseParser then. Also the previous setHitsExecutionNeeded wasn't useful, it can be removed as well, given that once there is a parsed ext section, it will become a builder that can be retrieved by the sub fetch phase. The sub fetch phase is responsible for doing nothing in case the builder is not set, meaning that the fetch sub phase is plugged in but the request didn't have the corresponding section. --- .../search/SearchParseElement.java | 31 ------------- .../elasticsearch/search/SearchService.java | 5 +- .../search/fetch/FetchSubPhaseContext.java | 46 ------------------- .../search/fetch/FetchSubPhaseParser.java | 4 +- .../search/internal/DefaultSearchContext.java | 12 ++--- .../internal/FilteredSearchContext.java | 9 ++-- .../search/internal/SearchContext.java | 6 +-- .../search/fetch/FetchSubPhasePluginIT.java | 43 ++++++++--------- .../elasticsearch/test/TestSearchContext.java | 12 ++--- 9 files changed, 39 insertions(+), 129 deletions(-) delete mode 100644 core/src/main/java/org/elasticsearch/search/SearchParseElement.java delete mode 100644 core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseContext.java diff --git a/core/src/main/java/org/elasticsearch/search/SearchParseElement.java b/core/src/main/java/org/elasticsearch/search/SearchParseElement.java deleted file mode 100644 index 9bf680deb55..00000000000 --- a/core/src/main/java/org/elasticsearch/search/SearchParseElement.java +++ /dev/null @@ -1,31 +0,0 @@ -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.elasticsearch.search; - -import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.search.internal.SearchContext; - -/** - * - */ -public interface SearchParseElement { - - void parse(XContentParser parser, SearchContext context) throws Exception; -} diff --git a/core/src/main/java/org/elasticsearch/search/SearchService.java b/core/src/main/java/org/elasticsearch/search/SearchService.java index 8ecc997dbb7..5ed2f62d9cc 100644 --- a/core/src/main/java/org/elasticsearch/search/SearchService.java +++ b/core/src/main/java/org/elasticsearch/search/SearchService.java @@ -63,7 +63,6 @@ import org.elasticsearch.search.dfs.DfsPhase; import org.elasticsearch.search.dfs.DfsSearchResult; import org.elasticsearch.search.fetch.FetchPhase; import org.elasticsearch.search.fetch.FetchSearchResult; -import org.elasticsearch.search.fetch.FetchSubPhaseContext; import org.elasticsearch.search.fetch.FetchSubPhaseParser; import org.elasticsearch.search.fetch.QueryFetchSearchResult; import org.elasticsearch.search.fetch.ScrollQueryFetchSearchResult; @@ -774,8 +773,8 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv throw new SearchParseException(context, "Unknown element [" + currentFieldName + "] in [ext]", extParser.getTokenLocation()); } else { - FetchSubPhaseContext fetchSubPhaseContext = fetchSubPhaseParser.parse(extParser); - context.putFetchSubPhaseContext(currentFieldName, fetchSubPhaseContext); + Object fetchSubPhaseBuilder = fetchSubPhaseParser.parse(extParser); + context.putFetchSubPhaseBuilder(currentFieldName, fetchSubPhaseBuilder); } } } diff --git a/core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseContext.java b/core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseContext.java deleted file mode 100644 index f2aa1b88138..00000000000 --- a/core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseContext.java +++ /dev/null @@ -1,46 +0,0 @@ -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.elasticsearch.search.fetch; - -/** - * All configuration and context needed by the FetchSubPhase to execute on hits. - * The only required information in this base class is whether or not the sub phase needs to be run at all. - * It can be extended by FetchSubPhases to hold information the phase needs to execute on hits. - */ -public abstract class FetchSubPhaseContext { - - // This is to store if the FetchSubPhase should be executed at all. - private boolean hitExecutionNeeded = false; - - /** - * Set if this phase should be executed at all. - */ - public void setHitExecutionNeeded(boolean hitExecutionNeeded) { - this.hitExecutionNeeded = hitExecutionNeeded; - } - - /** - * Returns if this phase be executed at all. - */ - public boolean hitExecutionNeeded() { - return hitExecutionNeeded; - } - -} diff --git a/core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseParser.java b/core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseParser.java index c88b1995688..1fddf442d2d 100644 --- a/core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseParser.java +++ b/core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseParser.java @@ -24,7 +24,7 @@ import org.elasticsearch.common.xcontent.XContentParser; /** * Parser for the ext section of a search request, which can hold custom fetch sub phase */ -public interface FetchSubPhaseParser { +public interface FetchSubPhaseParser { - Context parse(XContentParser parser) throws Exception; + Object parse(XContentParser parser) throws Exception; } diff --git a/core/src/main/java/org/elasticsearch/search/internal/DefaultSearchContext.java b/core/src/main/java/org/elasticsearch/search/internal/DefaultSearchContext.java index bf968e8f0cd..35f472da46d 100644 --- a/core/src/main/java/org/elasticsearch/search/internal/DefaultSearchContext.java +++ b/core/src/main/java/org/elasticsearch/search/internal/DefaultSearchContext.java @@ -58,7 +58,6 @@ import org.elasticsearch.search.aggregations.SearchContextAggregations; import org.elasticsearch.search.dfs.DfsSearchResult; import org.elasticsearch.search.fetch.FetchPhase; import org.elasticsearch.search.fetch.FetchSearchResult; -import org.elasticsearch.search.fetch.FetchSubPhaseContext; import org.elasticsearch.search.fetch.StoredFieldsContext; import org.elasticsearch.search.fetch.subphase.DocValueFieldsContext; import org.elasticsearch.search.fetch.subphase.FetchSourceContext; @@ -146,7 +145,7 @@ public class DefaultSearchContext extends SearchContext { private volatile long lastAccessTime = -1; private Profilers profilers; - private final Map subPhaseContexts = new HashMap<>(); + private final Map subPhaseBuilders = new HashMap<>(); private final Map, Collector> queryCollectors = new HashMap<>(); private final QueryShardContext queryShardContext; private FetchPhase fetchPhase; @@ -386,14 +385,13 @@ public class DefaultSearchContext extends SearchContext { } @Override - @SuppressWarnings("unchecked") - public SubPhaseContext getFetchSubPhaseContext(String name) { - return (SubPhaseContext) subPhaseContexts.get(name); + public Object getFetchSubPhaseBuilder(String name) { + return subPhaseBuilders.get(name); } @Override - public void putFetchSubPhaseContext(String name, SubPhaseContext subPhaseContext) { - subPhaseContexts.put(name, subPhaseContext); + public void putFetchSubPhaseBuilder(String name, Object fetchSubPhaseBuilder) { + subPhaseBuilders.put(name, fetchSubPhaseBuilder); } @Override diff --git a/core/src/main/java/org/elasticsearch/search/internal/FilteredSearchContext.java b/core/src/main/java/org/elasticsearch/search/internal/FilteredSearchContext.java index 250721ba27d..afd14ce07ab 100644 --- a/core/src/main/java/org/elasticsearch/search/internal/FilteredSearchContext.java +++ b/core/src/main/java/org/elasticsearch/search/internal/FilteredSearchContext.java @@ -43,7 +43,6 @@ import org.elasticsearch.search.aggregations.SearchContextAggregations; import org.elasticsearch.search.dfs.DfsSearchResult; import org.elasticsearch.search.fetch.FetchPhase; import org.elasticsearch.search.fetch.FetchSearchResult; -import org.elasticsearch.search.fetch.FetchSubPhaseContext; import org.elasticsearch.search.fetch.StoredFieldsContext; import org.elasticsearch.search.fetch.subphase.FetchSourceContext; import org.elasticsearch.search.fetch.subphase.InnerHitsContext; @@ -511,13 +510,13 @@ public abstract class FilteredSearchContext extends SearchContext { } @Override - public SubPhaseContext getFetchSubPhaseContext(String name) { - return in.getFetchSubPhaseContext(name); + public void putFetchSubPhaseBuilder(String name, Object fetchSubPhaseBuilder) { + in.putFetchSubPhaseBuilder(name, fetchSubPhaseBuilder); } @Override - public void putFetchSubPhaseContext(String name, SubPhaseContext subPhaseContext) { - in.putFetchSubPhaseContext(name, subPhaseContext); + public Object getFetchSubPhaseBuilder(String name) { + return in.getFetchSubPhaseBuilder(name); } @Override diff --git a/core/src/main/java/org/elasticsearch/search/internal/SearchContext.java b/core/src/main/java/org/elasticsearch/search/internal/SearchContext.java index d92fc79dd32..4520c866602 100644 --- a/core/src/main/java/org/elasticsearch/search/internal/SearchContext.java +++ b/core/src/main/java/org/elasticsearch/search/internal/SearchContext.java @@ -49,7 +49,6 @@ import org.elasticsearch.search.aggregations.SearchContextAggregations; import org.elasticsearch.search.dfs.DfsSearchResult; import org.elasticsearch.search.fetch.FetchPhase; import org.elasticsearch.search.fetch.FetchSearchResult; -import org.elasticsearch.search.fetch.FetchSubPhaseContext; import org.elasticsearch.search.fetch.StoredFieldsContext; import org.elasticsearch.search.fetch.subphase.DocValueFieldsContext; import org.elasticsearch.search.fetch.subphase.FetchSourceContext; @@ -185,10 +184,9 @@ public abstract class SearchContext extends AbstractRefCounted implements Releas public abstract SearchContext aggregations(SearchContextAggregations aggregations); - public abstract void putFetchSubPhaseContext(String name, - SubPhaseContext subPhaseContext); + public abstract void putFetchSubPhaseBuilder(String name, Object fetchSubPhaseBuilder); - public abstract SubPhaseContext getFetchSubPhaseContext(String name); + public abstract Object getFetchSubPhaseBuilder(String name); public abstract SearchContextHighlight highlight(); diff --git a/core/src/test/java/org/elasticsearch/search/fetch/FetchSubPhasePluginIT.java b/core/src/test/java/org/elasticsearch/search/fetch/FetchSubPhasePluginIT.java index 7f4b0911941..a7e2988f16e 100644 --- a/core/src/test/java/org/elasticsearch/search/fetch/FetchSubPhasePluginIT.java +++ b/core/src/test/java/org/elasticsearch/search/fetch/FetchSubPhasePluginIT.java @@ -26,6 +26,7 @@ import org.apache.lucene.util.BytesRef; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.termvectors.TermVectorsRequest; import org.elasticsearch.action.termvectors.TermVectorsResponse; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.logging.ESLoggerFactory; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; @@ -62,6 +63,7 @@ public class FetchSubPhasePluginIT extends ESIntegTestCase { return Collections.singletonList(FetchTermVectorsPlugin.class); } + @SuppressWarnings("unchecked") public void testPlugin() throws Exception { client().admin() .indices() @@ -104,22 +106,20 @@ public class FetchSubPhasePluginIT extends ESIntegTestCase { } public static final class TermVectorsFetchSubPhase implements FetchSubPhase { - - public static final String NAME = "term_vectors_fetch"; + private static final String NAME = "term_vectors_fetch"; @Override public Map parsers() { - return singletonMap("term_vectors_fetch", new TermVectorsFetchParser()); + return singletonMap(NAME, new TermVectorsFetchParser()); } @Override public void hitExecute(SearchContext context, HitContext hitContext) { - TermVectorsFetchContext fetchSubPhaseContext = context.getFetchSubPhaseContext(NAME); - if (fetchSubPhaseContext.hitExecutionNeeded() == false) { + TermVectorsFetchBuilder fetchSubPhaseBuilder = (TermVectorsFetchBuilder)context.getFetchSubPhaseBuilder(NAME); + if (fetchSubPhaseBuilder == null) { return; } - String field = fetchSubPhaseContext.getField(); - + String field = fetchSubPhaseBuilder.getField(); if (hitContext.hit().fieldsOrNull() == null) { hitContext.hit().fields(new HashMap<>()); } @@ -145,32 +145,27 @@ public class FetchSubPhasePluginIT extends ESIntegTestCase { } } - public static class TermVectorsFetchParser implements FetchSubPhaseParser { - + public static class TermVectorsFetchParser implements FetchSubPhaseParser { @Override - public TermVectorsFetchContext parse(XContentParser parser) throws Exception { - // this is to make sure that the SubFetchPhase knows it should execute - TermVectorsFetchContext termVectorsFetchContext = new TermVectorsFetchContext(); - termVectorsFetchContext.setHitExecutionNeeded(true); + public TermVectorsFetchBuilder parse(XContentParser parser) throws Exception { + String field; XContentParser.Token token = parser.currentToken(); if (token == XContentParser.Token.VALUE_STRING) { - String fieldName = parser.text(); - termVectorsFetchContext.setField(fieldName); + field = parser.text(); } else { - throw new IllegalStateException("Expected a VALUE_STRING but got " + token); + throw new ParsingException(parser.getTokenLocation(), "Expected a VALUE_STRING but got " + token); } - return termVectorsFetchContext; + if (field == null) { + throw new ParsingException(parser.getTokenLocation(), "no fields specified for " + TermVectorsFetchSubPhase.NAME); + } + return new TermVectorsFetchBuilder(field); } } - public static class TermVectorsFetchContext extends FetchSubPhaseContext { + public static class TermVectorsFetchBuilder { + private final String field; - private String field = null; - - public TermVectorsFetchContext() { - } - - public void setField(String field) { + private TermVectorsFetchBuilder(String field) { this.field = field; } diff --git a/test/framework/src/main/java/org/elasticsearch/test/TestSearchContext.java b/test/framework/src/main/java/org/elasticsearch/test/TestSearchContext.java index 67c3ba5b55f..a520b144df1 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/TestSearchContext.java +++ b/test/framework/src/main/java/org/elasticsearch/test/TestSearchContext.java @@ -44,7 +44,6 @@ import org.elasticsearch.search.aggregations.SearchContextAggregations; import org.elasticsearch.search.dfs.DfsSearchResult; import org.elasticsearch.search.fetch.FetchPhase; import org.elasticsearch.search.fetch.FetchSearchResult; -import org.elasticsearch.search.fetch.FetchSubPhaseContext; import org.elasticsearch.search.fetch.StoredFieldsContext; import org.elasticsearch.search.fetch.subphase.DocValueFieldsContext; import org.elasticsearch.search.fetch.subphase.FetchSourceContext; @@ -90,7 +89,7 @@ public class TestSearchContext extends SearchContext { private SearchContextAggregations aggregations; private final long originNanoTime = System.nanoTime(); - private final Map subPhaseContexts = new HashMap<>(); + private final Map subPhaseContexts = new HashMap<>(); public TestSearchContext(ThreadPool threadPool, BigArrays bigArrays, ScriptService scriptService, IndexService indexService) { super(ParseFieldMatcher.STRICT); @@ -197,14 +196,13 @@ public class TestSearchContext extends SearchContext { } @Override - @SuppressWarnings("unchecked") - public SubPhaseContext getFetchSubPhaseContext(String name) { - return (SubPhaseContext) subPhaseContexts.get(name); + public void putFetchSubPhaseBuilder(String name, Object fetchSubPhaseBuilder) { + subPhaseContexts.put(name, fetchSubPhaseBuilder); } @Override - public void putFetchSubPhaseContext(String name, SubPhaseContext subPhaseContext) { - subPhaseContexts.put(name, subPhaseContext); + public Object getFetchSubPhaseBuilder(String name) { + return subPhaseContexts.get(name); } @Override