From e20f8d04bf0d656c7c1a5fd0b0e5ecb135f8704c Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Fri, 18 May 2018 11:49:17 +0200 Subject: [PATCH 1/4] Deprecates completion query without context This change deprecates completion queries without context that target a context enabled completion field. Querying without context degrades the search performance considerably (even when the number of indexed contexts is low). This commit targets master but the deprecation will take place in 6.x and the functionality will be removed in 7 in a follow up. Closes #29222 --- .../suggesters/context-suggest.asciidoc | 6 ++-- .../rest-api-spec/test/suggest/30_context.yml | 34 +++++++++++++++++++ .../CompletionSuggestionBuilder.java | 23 +++++++++---- 3 files changed, 53 insertions(+), 10 deletions(-) diff --git a/docs/reference/search/suggesters/context-suggest.asciidoc b/docs/reference/search/suggesters/context-suggest.asciidoc index 9226c29b9ad6e..e8b092498fe52 100644 --- a/docs/reference/search/suggesters/context-suggest.asciidoc +++ b/docs/reference/search/suggesters/context-suggest.asciidoc @@ -156,9 +156,9 @@ POST place/_search?pretty // CONSOLE // TEST[continued] -NOTE: When no categories are provided at query-time, all indexed documents are considered. -Querying with no categories on a category enabled completion field should be avoided, as it -will degrade search performance. +Note: deprecated[7.0.0, When no categories are provided at query-time, all indexed documents are considered. +Querying with no categories on a category enabled completion field is deprecated and will be removed in the next major release +as it degrades search performance considerably.] Suggestions with certain categories can be boosted higher than others. The following filters suggestions by categories and additionally boosts diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/30_context.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/30_context.yml index f0d97382eeb8e..615319cb65a35 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/30_context.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/30_context.yml @@ -349,3 +349,37 @@ setup: - length: { suggest.result: 1 } - length: { suggest.result.0.options: 1 } - match: { suggest.result.0.options.0.text: "foo" } + +--- +"Querying without contexts is deprecated": + - skip: + version: " - 6.99.99" + reason: this feature was deprecated in 7.0 + features: "warnings" + + - do: + index: + index: test + type: test + id: 1 + body: + suggest_context: + input: "foo" + contexts: + color: "red" + + - do: + indices.refresh: {} + + - do: + warnings: + - "The ability to query with no context on a context enabled completion field is deprecated and will be removed in the next major release." + search: + body: + suggest: + result: + text: "foo" + completion: + field: suggest_context + + - length: { suggest.result: 1 } diff --git a/server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java b/server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java index 3102ddd0e7787..3a260c8545306 100644 --- a/server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java @@ -24,6 +24,8 @@ import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.logging.DeprecationLogger; +import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.unit.Fuzziness; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; @@ -57,6 +59,10 @@ * indexing. */ public class CompletionSuggestionBuilder extends SuggestionBuilder { + + private static final DeprecationLogger DEPRECATION_LOGGER = + new DeprecationLogger(Loggers.getLogger(CompletionSuggestionBuilder.class)); + private static final XContentType CONTEXT_BYTES_XCONTENT_TYPE = XContentType.JSON; static final String SUGGESTION_NAME = "completion"; static final ParseField CONTEXTS_FIELD = new ParseField("contexts", "context"); @@ -298,16 +304,19 @@ public SuggestionContext build(QueryShardContext context) throws IOException { if (mappedFieldType == null || mappedFieldType instanceof CompletionFieldMapper.CompletionFieldType == false) { throw new IllegalArgumentException("Field [" + suggestionContext.getField() + "] is not a completion suggest field"); } - if (mappedFieldType instanceof CompletionFieldMapper.CompletionFieldType) { - CompletionFieldMapper.CompletionFieldType type = (CompletionFieldMapper.CompletionFieldType) mappedFieldType; - suggestionContext.setFieldType(type); - if (type.hasContextMappings() && contextBytes != null) { + CompletionFieldMapper.CompletionFieldType type = (CompletionFieldMapper.CompletionFieldType) mappedFieldType; + suggestionContext.setFieldType(type); + if (type.hasContextMappings()) { + if (contextBytes == null) { + DEPRECATION_LOGGER.deprecated("The ability to query with no context on a context enabled completion field is deprecated " + + "and will be removed in the next major release."); + } else { Map> queryContexts = parseContextBytes(contextBytes, - context.getXContentRegistry(), type.getContextMappings()); + context.getXContentRegistry(), type.getContextMappings()); suggestionContext.setQueryContexts(queryContexts); - } else if (contextBytes != null) { - throw new IllegalArgumentException("suggester [" + type.name() + "] doesn't expect any context"); } + } else if (contextBytes != null) { + throw new IllegalArgumentException("suggester [" + type.name() + "] doesn't expect any context"); } assert suggestionContext.getFieldType() != null : "no completion field type set"; return suggestionContext; From 0696ec403ae5442a3e328d35d76530c9b8b6a0fe Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Tue, 22 May 2018 11:03:57 +0200 Subject: [PATCH 2/4] fix rest tests --- .../rest-api-spec/test/suggest/30_context.yml | 14 -------------- .../rest-api-spec/test/suggest/40_typed_keys.yml | 6 ++++++ 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/30_context.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/30_context.yml index 615319cb65a35..c783f9fe938ac 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/30_context.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/30_context.yml @@ -336,20 +336,6 @@ setup: - length: { suggest.result.0.options: 1 } - match: { suggest.result.0.options.0.text: "foo" } - - do: - search: - body: - suggest: - result: - text: "foo" - completion: - skip_duplicates: true - field: suggest_context - - - length: { suggest.result: 1 } - - length: { suggest.result.0.options: 1 } - - match: { suggest.result.0.options.0.text: "foo" } - --- "Querying without contexts is deprecated": - skip: diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/40_typed_keys.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/40_typed_keys.yml index dffc1fdd7702d..aca51fcf77124 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/40_typed_keys.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/40_typed_keys.yml @@ -31,8 +31,14 @@ setup: --- "Test typed keys parameter for suggesters": + - skip: + version: " - 6.99.99" + reason: queying a context suggester with no context was deprecated in 7.0 + features: "warnings" - do: + warnings: + - "The ability to query with no context on a context enabled completion field is deprecated and will be removed in the next major release." search: typed_keys: true body: From f5edec43302eafc2edfea583b93e0b0996e5dc9a Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Tue, 22 May 2018 21:37:02 +0200 Subject: [PATCH 3/4] After review This commit moves the deprecation warning in order to ensure that all cases of empty contexts are handled. This change also deprecates the indexation of a context completion field without contexts. This is needed because these suggestions are not reachable anymore if querying without context is removed. --- .../suggesters/context-suggest.asciidoc | 4 ++ .../rest-api-spec/test/suggest/30_context.yml | 46 ++++++++++++++++++- .../index/mapper/CompletionFieldMapper.java | 1 - .../CompletionSuggestionBuilder.java | 22 +++------ .../completion/context/ContextMappings.java | 16 +++++++ 5 files changed, 72 insertions(+), 17 deletions(-) diff --git a/docs/reference/search/suggesters/context-suggest.asciidoc b/docs/reference/search/suggesters/context-suggest.asciidoc index e8b092498fe52..2b522062ec06c 100644 --- a/docs/reference/search/suggesters/context-suggest.asciidoc +++ b/docs/reference/search/suggesters/context-suggest.asciidoc @@ -84,6 +84,10 @@ PUT place_path_category NOTE: Adding context mappings increases the index size for completion field. The completion index is entirely heap resident, you can monitor the completion field index size using <>. +NOTE: deprecated[7.0.0, Indexing a suggestion without context on a context enabled completion field is deprecated +and will be removed in the next major release. If you want to index a suggestion that matches all contexts you should +add a special context for it.] + [[suggester-context-category]] [float] ==== Category Context diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/30_context.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/30_context.yml index c783f9fe938ac..dfb849fff5700 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/30_context.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/30_context.yml @@ -337,7 +337,7 @@ setup: - match: { suggest.result.0.options.0.text: "foo" } --- -"Querying without contexts is deprecated": +"Indexing and Querying without contexts is deprecated": - skip: version: " - 6.99.99" reason: this feature was deprecated in 7.0 @@ -353,6 +353,21 @@ setup: input: "foo" contexts: color: "red" + suggest_multi_contexts: + input: "bar" + contexts: + color: "blue" + + - do: + warnings: + - "The ability to index a suggestion with no context on a context enabled completion field is deprecated and will be removed in the next major release." + index: + index: test + type: test + id: 2 + body: + suggest_context: + input: "foo" - do: indices.refresh: {} @@ -369,3 +384,32 @@ setup: field: suggest_context - length: { suggest.result: 1 } + + - do: + warnings: + - "The ability to query with no context on a context enabled completion field is deprecated and will be removed in the next major release." + search: + body: + suggest: + result: + text: "foo" + completion: + field: suggest_context + contexts: {} + + - length: { suggest.result: 1 } + + - do: + warnings: + - "The ability to query with no context on a context enabled completion field is deprecated and will be removed in the next major release." + search: + body: + suggest: + result: + text: "foo" + completion: + field: suggest_multi_contexts + contexts: + location: [] + + - length: { suggest.result: 1 } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java index af50dfbcb4e18..e8b07239446bb 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java @@ -83,7 +83,6 @@ * for query-time filtering and boosting (see {@link ContextMappings} */ public class CompletionFieldMapper extends FieldMapper implements ArrayValueMapperParser { - public static final String CONTENT_TYPE = "completion"; public static class Defaults { diff --git a/server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java b/server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java index 3a260c8545306..25f2f7fa382fa 100644 --- a/server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestionBuilder.java @@ -24,8 +24,6 @@ import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.logging.DeprecationLogger; -import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.unit.Fuzziness; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; @@ -60,9 +58,6 @@ */ public class CompletionSuggestionBuilder extends SuggestionBuilder { - private static final DeprecationLogger DEPRECATION_LOGGER = - new DeprecationLogger(Loggers.getLogger(CompletionSuggestionBuilder.class)); - private static final XContentType CONTEXT_BYTES_XCONTENT_TYPE = XContentType.JSON; static final String SUGGESTION_NAME = "completion"; static final ParseField CONTEXTS_FIELD = new ParseField("contexts", "context"); @@ -304,19 +299,16 @@ public SuggestionContext build(QueryShardContext context) throws IOException { if (mappedFieldType == null || mappedFieldType instanceof CompletionFieldMapper.CompletionFieldType == false) { throw new IllegalArgumentException("Field [" + suggestionContext.getField() + "] is not a completion suggest field"); } - CompletionFieldMapper.CompletionFieldType type = (CompletionFieldMapper.CompletionFieldType) mappedFieldType; - suggestionContext.setFieldType(type); - if (type.hasContextMappings()) { - if (contextBytes == null) { - DEPRECATION_LOGGER.deprecated("The ability to query with no context on a context enabled completion field is deprecated " + - "and will be removed in the next major release."); - } else { + if (mappedFieldType instanceof CompletionFieldMapper.CompletionFieldType) { + CompletionFieldMapper.CompletionFieldType type = (CompletionFieldMapper.CompletionFieldType) mappedFieldType; + suggestionContext.setFieldType(type); + if (type.hasContextMappings() && contextBytes != null) { Map> queryContexts = parseContextBytes(contextBytes, - context.getXContentRegistry(), type.getContextMappings()); + context.getXContentRegistry(), type.getContextMappings()); suggestionContext.setQueryContexts(queryContexts); + } else if (contextBytes != null) { + throw new IllegalArgumentException("suggester [" + type.name() + "] doesn't expect any context"); } - } else if (contextBytes != null) { - throw new IllegalArgumentException("suggester [" + type.name() + "] doesn't expect any context"); } assert suggestionContext.getFieldType() != null : "no completion field type set"; return suggestionContext; diff --git a/server/src/main/java/org/elasticsearch/search/suggest/completion/context/ContextMappings.java b/server/src/main/java/org/elasticsearch/search/suggest/completion/context/ContextMappings.java index a0a6ce59f0d9d..4d6b53296f157 100644 --- a/server/src/main/java/org/elasticsearch/search/suggest/completion/context/ContextMappings.java +++ b/server/src/main/java/org/elasticsearch/search/suggest/completion/context/ContextMappings.java @@ -25,6 +25,8 @@ import org.apache.lucene.util.CharsRefBuilder; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.Version; +import org.elasticsearch.common.logging.DeprecationLogger; +import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.index.mapper.CompletionFieldMapper; @@ -51,6 +53,10 @@ * for a {@link CompletionFieldMapper} */ public class ContextMappings implements ToXContent { + + private static final DeprecationLogger DEPRECATION_LOGGER = + new DeprecationLogger(Loggers.getLogger(ContextMappings.class)); + private final List contextMappings; private final Map contextNameMap; @@ -143,6 +149,10 @@ protected Iterable contexts() { scratch.setLength(1); } } + if (typedContexts.isEmpty()) { + DEPRECATION_LOGGER.deprecated("The ability to index a suggestion with no context on a context enabled completion field" + + " is deprecated and will be removed in the next major release."); + } return typedContexts; } } @@ -156,6 +166,7 @@ protected Iterable contexts() { */ public ContextQuery toContextQuery(CompletionQuery query, Map> queryContexts) { ContextQuery typedContextQuery = new ContextQuery(query); + boolean hasContext = false; if (queryContexts.isEmpty() == false) { CharsRefBuilder scratch = new CharsRefBuilder(); scratch.grow(1); @@ -169,10 +180,15 @@ public ContextQuery toContextQuery(CompletionQuery query, Map Date: Wed, 23 May 2018 09:59:18 +0200 Subject: [PATCH 4/4] fix rest test --- .../resources/rest-api-spec/test/suggest/40_typed_keys.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/40_typed_keys.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/40_typed_keys.yml index aca51fcf77124..604be9ec99e00 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/40_typed_keys.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/suggest/40_typed_keys.yml @@ -19,7 +19,9 @@ setup: "type" : "category" - do: - bulk: + warnings: + - "The ability to index a suggestion with no context on a context enabled completion field is deprecated and will be removed in the next major release." + bulk: refresh: true index: test type: test