Add parsing from xContent to PhraseSuggestionBuilder#16354
Conversation
12bcf82 to
c66b050
Compare
2bfd196 to
de5a7dd
Compare
There was a problem hiding this comment.
I'm not sure we should add suggesters to this class as it means that anywhere we need to use queries (percolator, aggs, etc.) we need to have suggesters too. I think the suggesters should have their own registry class instead
|
@cbuescher I am concerned about this change making suggesters required everywhere we have queries. Queries are used a lot outside of the Search API and I think it will make other modules (e.g. percolator and aggs) difficult to maintain if they need to pass in suggesters when they don't use them. Personally I think the suggester should have a separate registry like the Aggregations do. |
|
@colings86 thanks, I will take a look at alternatives, Suggesters already is the central registry, I only need a way to access it from the SuggestionBuilder fromXContent method. Maybe directly adding it to the context there would be an option. Any other ideas welcome. |
|
Colin has a good point, would it be possible to treat Suggesters as the registry and instead of adding it to the existing IndicesQueriesRegistry, inject that directly to the parse context? That said the naming is a bit off at the moment, I am not clear on whether QueryParseContext will become SearchParseContext at some point . |
ce2d247 to
ebc36e0
Compare
|
@colings86 @javanna thanks for the comments, I tried to solve the suggestion builder lookup differently now. The lookup is needed in the common parsing code for fields all SuggestionBuilder share, so initially I put it in the abstract superclass, but by putting it into SuggestParseElement it can directly use the registry that is already wired there. The downside is, we don't have the parsing code in the builder, but thats also not true for the queries. Also the duplicated code we temporarily have in SuggestParseElement will go away after the complete refactoring (also we don't need to call it SuggestParseElement then). |
There was a problem hiding this comment.
At this point, it is still possible that the suggestionBuilder could be null, so it would be better to have a check here to throw an exception if suggestionBuilder is null here.
There was a problem hiding this comment.
good point, added that check
d979e36 to
508ca0a
Compare
|
I went a step back and optionally added the suggesters registry I need for parsing to the QueryParseContext directly. That way, when we need it can be add it (e.g. in SearchService when starting the whole parsing chain) and its not part of IndicesQueriesRegistry. |
c97f39f to
211e64b
Compare
c53640c to
e5f3248
Compare
|
LGTM |
For the ongoing search refactoring (elastic#10217) the PhraseSuggestionBuilder gets a way of parsing from xContent that will eventually replace the current SuggestParseElement. This PR adds the fromXContent method to the PhraseSuggestionBuilder and also adds parsing code for the common suggestion parameters to SuggestionBuilder. Also adding links from the Suggester implementations registeres in the Suggesters registry to the corresponding prototype that is going to be used for parsing once the refactoring is done and we switch from parsing on shard to parsing on coordinating node.
e5f3248 to
2ae6420
Compare
This adds parsing from xContent to PhraseSuggestionBuilder and changes properties of PhraseSuggestionBuilder to using default values form PhraseSuggestionContext where possible.
This adds parsing from xContent to PhraseSuggestionBuilder and changes properties of PhraseSuggestionBuilder to using default values form PhraseSuggestionContext where possible. Also adding access to the global Suggesters via QueryParseContext so we can retrieve the right prototype for parsing the suggestions.