Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

serialize suggestion responses as named writeables #30284

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
64369c8
serialize suggestion responses as named writeables
andyb-elastic Mar 10, 2018
990d546
checkstyle fixes
andyb-elastic May 1, 2018
09b3ccc
Merge branch 'master' into feature-plugin-suggester-named-writeable-a…
andyb-elastic Jul 18, 2018
4f695a7
wip suggest update tests with new option usage
andyb-elastic Jul 18, 2018
155df07
wip suggest only one suggestion reader
andyb-elastic Jul 18, 2018
3add813
rest tests for custom suggester example plugin
andyb-elastic Jul 25, 2018
f062443
checkstyle fixes
andyb-elastic Jul 25, 2018
5b8fa25
fill in toXContent support for custom fields - change Entry and Option
andyb-elastic Jul 25, 2018
4e5ff17
remove custom suggester IT from server
andyb-elastic Jul 26, 2018
80ce511
Merge branch 'master' into feature-plugin-suggester-named-writeable-a…
andyb-elastic Jul 26, 2018
69163ba
use an empty test suggester impl in SearchModuleTests
andyb-elastic Jul 26, 2018
bcd3704
backwards compatiblity corrections
andyb-elastic Jul 27, 2018
87bddd1
make release note more clear
andyb-elastic Jul 27, 2018
a7c3f01
Merge branch 'master' into feature-plugin-suggester-named-writeable-a…
andyb-elastic Jul 30, 2018
2923532
use innerReadFrom innerWriteTo for old nodes
andyb-elastic Jul 31, 2018
0059b61
serialization shim for TermSuggestion
andyb-elastic Jul 31, 2018
a2b7bef
wip round triup works but only because it's not equalsing
andyb-elastic Aug 2, 2018
df532f1
serialization tests pass
andyb-elastic Aug 2, 2018
ee7efb4
remove commented out methods
andyb-elastic Aug 3, 2018
b77ed84
Merge branch 'master' into feature-plugin-suggester-named-writeable-a…
andyb-elastic Aug 3, 2018
4603a0d
Merge branch 'master' into feature-plugin-suggester-named-writeable-a…
andyb-elastic Aug 3, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,11 @@
import org.elasticsearch.search.aggregations.pipeline.derivative.ParsedDerivative;
import org.elasticsearch.search.suggest.Suggest;
import org.elasticsearch.search.suggest.completion.CompletionSuggestion;
import org.elasticsearch.search.suggest.completion.CompletionSuggestionBuilder;
import org.elasticsearch.search.suggest.phrase.PhraseSuggestion;
import org.elasticsearch.search.suggest.phrase.PhraseSuggestionBuilder;
import org.elasticsearch.search.suggest.term.TermSuggestion;
import org.elasticsearch.search.suggest.term.TermSuggestionBuilder;

import java.io.Closeable;
import java.io.IOException;
Expand Down Expand Up @@ -707,11 +710,11 @@ static List<NamedXContentRegistry.Entry> getDefaultNamedXContents() {
List<NamedXContentRegistry.Entry> entries = map.entrySet().stream()
.map(entry -> new NamedXContentRegistry.Entry(Aggregation.class, new ParseField(entry.getKey()), entry.getValue()))
.collect(Collectors.toList());
entries.add(new NamedXContentRegistry.Entry(Suggest.Suggestion.class, new ParseField(TermSuggestion.NAME),
entries.add(new NamedXContentRegistry.Entry(Suggest.Suggestion.class, new ParseField(TermSuggestionBuilder.SUGGESTION_NAME),
(parser, context) -> TermSuggestion.fromXContent(parser, (String)context)));
entries.add(new NamedXContentRegistry.Entry(Suggest.Suggestion.class, new ParseField(PhraseSuggestion.NAME),
entries.add(new NamedXContentRegistry.Entry(Suggest.Suggestion.class, new ParseField(PhraseSuggestionBuilder.SUGGESTION_NAME),
(parser, context) -> PhraseSuggestion.fromXContent(parser, (String)context)));
entries.add(new NamedXContentRegistry.Entry(Suggest.Suggestion.class, new ParseField(CompletionSuggestion.NAME),
entries.add(new NamedXContentRegistry.Entry(Suggest.Suggestion.class, new ParseField(CompletionSuggestionBuilder.SUGGESTION_NAME),
(parser, context) -> CompletionSuggestion.fromXContent(parser, (String)context)));
return entries;
}
Expand Down
53 changes: 41 additions & 12 deletions server/src/main/java/org/elasticsearch/plugins/SearchPlugin.java
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import org.elasticsearch.search.fetch.subphase.highlight.Highlighter;
import org.elasticsearch.search.rescore.RescorerBuilder;
import org.elasticsearch.search.rescore.Rescorer;
import org.elasticsearch.search.suggest.Suggest;
import org.elasticsearch.search.suggest.Suggester;
import org.elasticsearch.search.suggest.SuggestionBuilder;

Expand Down Expand Up @@ -151,31 +152,59 @@ public ScoreFunctionSpec(String name, Writeable.Reader<T> reader, ScoreFunctionP
* Specification for a {@link Suggester}.
*/
class SuggesterSpec<T extends SuggestionBuilder<T>> extends SearchExtensionSpec<T, CheckedFunction<XContentParser, T, IOException>> {

private Map<String, Writeable.Reader<? extends Suggest.Suggestion>> suggestionReaders = new TreeMap<>();

/**
* Specification of custom {@link Suggester}.
*
* @param name holds the names by which this suggester might be parsed. The {@link ParseField#getPreferredName()} is special as it
* is the name by under which the reader is registered. So it is the name that the query should use as its
* {@link NamedWriteable#getWriteableName()} too.
* @param reader the reader registered for this suggester's builder. Typically a reference to a constructor that takes a
* is the name by under which the request builder and Suggestion response readers are registered. So it is the name that the
* query and Suggestion response should use as their {@link NamedWriteable#getWriteableName()} return values too.
* @param builderReader the reader registered for this suggester's builder. Typically a reference to a constructor that takes a
* {@link StreamInput}
* @param parser the parser the reads the query suggester from xcontent
* @param builderParser a parser that reads the suggester's builder from xcontent
* @param suggestionReader the reader registered for this suggester's Suggestion response. Typically a reference to a constructor
* that takes a {@link StreamInput}
*/
public SuggesterSpec(ParseField name, Writeable.Reader<T> reader, CheckedFunction<XContentParser, T, IOException> parser) {
super(name, reader, parser);
public SuggesterSpec(
ParseField name,
Writeable.Reader<T> builderReader,
CheckedFunction<XContentParser, T, IOException> builderParser,
Writeable.Reader<? extends Suggest.Suggestion> suggestionReader) {

super(name, builderReader, builderParser);
addSuggestionReader(name.getPreferredName(), suggestionReader);
}

/**
* Specification of custom {@link Suggester}.
*
* @param name the name by which this suggester might be parsed or deserialized. Make sure that the query builder returns this name
* for {@link NamedWriteable#getWriteableName()}.
* @param reader the reader registered for this suggester's builder. Typically a reference to a constructor that takes a
* @param name the name by which this suggester might be parsed or deserialized. Make sure that the query builder and Suggestion
* response reader return this name for {@link NamedWriteable#getWriteableName()}.
* @param builderReader the reader registered for this suggester's builder. Typically a reference to a constructor that takes a
* {@link StreamInput}
* @param parser the parser the reads the suggester builder from xcontent
* @param builderParser a parser that reads the suggester's builder from xcontent
* @param suggestionReader the reader registered for this suggester's Suggestion response. Typically a reference to a constructor
* that takes a {@link StreamInput}
*/
public SuggesterSpec(String name, Writeable.Reader<T> reader, CheckedFunction<XContentParser, T, IOException> parser) {
super(name, reader, parser);
public SuggesterSpec(
String name,
Writeable.Reader<T> builderReader,
CheckedFunction<XContentParser, T, IOException> builderParser,
Writeable.Reader<? extends Suggest.Suggestion> suggestionReader) {

super(name, builderReader, builderParser);
addSuggestionReader(name, suggestionReader);
}

public SuggesterSpec addSuggestionReader(String writeableName, Writeable.Reader<? extends Suggest.Suggestion> reader) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it needed to allow multiple readers ? Only one custom suggestion implementation should be needed here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking here was that it wouldn't be unreasonable for a suggester to return multiple types of suggestions in different situations. For example it looks like some aggregations do this if I'm not mistaken. But it makes sense and is simpler if they only use one, so I'll do that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

suggestionReaders.put(writeableName, reader);
return this;
}

public Map<String, Writeable.Reader<? extends Suggest.Suggestion>> getSuggestionReaders() {
return suggestionReaders;
}
}

Expand Down
21 changes: 18 additions & 3 deletions server/src/main/java/org/elasticsearch/search/SearchModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -242,13 +242,17 @@
import org.elasticsearch.search.sort.ScoreSortBuilder;
import org.elasticsearch.search.sort.ScriptSortBuilder;
import org.elasticsearch.search.sort.SortBuilder;
import org.elasticsearch.search.suggest.Suggest;
import org.elasticsearch.search.suggest.SuggestionBuilder;
import org.elasticsearch.search.suggest.completion.CompletionSuggestion;
import org.elasticsearch.search.suggest.completion.CompletionSuggestionBuilder;
import org.elasticsearch.search.suggest.phrase.Laplace;
import org.elasticsearch.search.suggest.phrase.LinearInterpolation;
import org.elasticsearch.search.suggest.phrase.PhraseSuggestion;
import org.elasticsearch.search.suggest.phrase.PhraseSuggestionBuilder;
import org.elasticsearch.search.suggest.phrase.SmoothingModel;
import org.elasticsearch.search.suggest.phrase.StupidBackoff;
import org.elasticsearch.search.suggest.term.TermSuggestion;
import org.elasticsearch.search.suggest.term.TermSuggestionBuilder;

import java.util.ArrayList;
Expand Down Expand Up @@ -577,9 +581,14 @@ public static void registerSmoothingModels(List<Entry> namedWriteables) {
private void registerSuggesters(List<SearchPlugin> plugins) {
registerSmoothingModels(namedWriteables);

registerSuggester(new SuggesterSpec<>("term", TermSuggestionBuilder::new, TermSuggestionBuilder::fromXContent));
registerSuggester(new SuggesterSpec<>("phrase", PhraseSuggestionBuilder::new, PhraseSuggestionBuilder::fromXContent));
registerSuggester(new SuggesterSpec<>("completion", CompletionSuggestionBuilder::new, CompletionSuggestionBuilder::fromXContent));
registerSuggester(new SuggesterSpec<>(TermSuggestionBuilder.SUGGESTION_NAME,
TermSuggestionBuilder::new, TermSuggestionBuilder::fromXContent, TermSuggestion::new));

registerSuggester(new SuggesterSpec<>(PhraseSuggestionBuilder.SUGGESTION_NAME,
PhraseSuggestionBuilder::new, PhraseSuggestionBuilder::fromXContent, PhraseSuggestion::new));

registerSuggester(new SuggesterSpec<>(CompletionSuggestionBuilder.SUGGESTION_NAME,
CompletionSuggestionBuilder::new, CompletionSuggestionBuilder::fromXContent, CompletionSuggestion::new));

registerFromPlugin(plugins, SearchPlugin::getSuggesters, this::registerSuggester);
}
Expand All @@ -589,6 +598,12 @@ private void registerSuggester(SuggesterSpec<?> suggester) {
SuggestionBuilder.class, suggester.getName().getPreferredName(), suggester.getReader()));
namedXContents.add(new NamedXContentRegistry.Entry(SuggestionBuilder.class, suggester.getName(),
suggester.getParser()));

for (Map.Entry<String, Writeable.Reader<? extends Suggest.Suggestion>> entry : suggester.getSuggestionReaders().entrySet()) {
String writeableName = entry.getKey();
Writeable.Reader<? extends Suggest.Suggestion> reader = entry.getValue();
namedWriteables.add(new NamedWriteableRegistry.Entry(Suggest.Suggestion.class, writeableName, reader));
}
}

private Map<String, Highlighter> setupHighlighters(Settings settings, List<SearchPlugin> plugins) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public InternalSearchResponse(StreamInput in) throws IOException {
super(
SearchHits.readSearchHits(in),
in.readBoolean() ? InternalAggregations.readAggregations(in) : null,
in.readBoolean() ? Suggest.readSuggest(in) : null,
in.readBoolean() ? new Suggest(in) : null,
in.readBoolean(),
in.readOptionalBoolean(),
in.readOptionalWriteable(SearchProfileShardResults::new),
Expand All @@ -62,7 +62,7 @@ public InternalSearchResponse(StreamInput in) throws IOException {
public void writeTo(StreamOutput out) throws IOException {
hits.writeTo(out);
out.writeOptionalStreamable((InternalAggregations)aggregations);
out.writeOptionalStreamable(suggest);
out.writeOptionalWriteable(suggest);
out.writeBoolean(timedOut);
out.writeOptionalBoolean(terminatedEarly);
out.writeOptionalWriteable(profileResults);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ public void readFromWithId(long id, StreamInput in) throws IOException {
pipelineAggregators = in.readNamedWriteableList(PipelineAggregator.class).stream().map(a -> (SiblingPipelineAggregator) a)
.collect(Collectors.toList());
if (in.readBoolean()) {
suggest = Suggest.readSuggest(in);
suggest = new Suggest(in);
}
searchTimedOut = in.readBoolean();
terminatedEarly = in.readOptionalBoolean();
Expand Down
Loading