Initial refactoring for phrase suggester #16241
Conversation
cbuescher
added
review
:Search Relevance/Suggesters
cbuescher
force-pushed
the
init-phraseSuggester-refactoring
branch
5 times, most recently
from
6afb09c to
8440320
Compare
| public CompletionSuggestionBuilder prefix(String prefix) { | ||
| super.setPrefix(prefix); | ||
| super.prefix(prefix); |
There was a problem hiding this comment.
Didn't we at some point want to move towards genuine getters and setters?
There was a problem hiding this comment.
Since almost all of the setters here are not get/set I wanted to make it consistent without too much changes. I'm happy to rename them all together in one go at a later stage though. Also if anybody else objects I can change this back of course, I just don't want to change all the non-get/set type ones at this point.
There was a problem hiding this comment.
Yeah - makes sense. Maybe track the final move to get/set in a separate issue?
There was a problem hiding this comment.
What was the motivation for wanting to make them genuine "get/set" methods?
|
Left a few comments. Feel free to ignore the ones about methods being empty. I didn't realize this wasn't going against master. |
cbuescher
force-pushed
the
init-phraseSuggester-refactoring
branch
2 times, most recently
from
025391f to
600a8f8
Compare
| @@ -91,6 +113,13 @@ public PhraseSuggestionBuilder separator(String separator) { | |||
| } | |||
|
|
|||
| /** | |||
| * get the maxErrirs setting | |||
There was a problem hiding this comment.
should be "get token separator"?
| public abstract class SuggestionBuilder<T extends SuggestionBuilder<T>> extends ToXContentToBytes implements NamedWriteable<T> { | ||
|
|
||
| protected final String name; | ||
| // TODO this seems mandatory and should be constructor arg |
|
@cbuescher LGTM ++ |
| return builder; | ||
| } | ||
|
|
||
| public String getSuggesterName() { |
There was a problem hiding this comment.
maybe we can make this private, I assume this is only for readability?
There was a problem hiding this comment.
Good point, will change that as well. And yes, currently only for readability. I hope we keep the same name constants as Id in the stream as well as for registering the parsers. That's what worked for queries at least.
|
@cbuescher LGTM. This looks great and thanks for the test infra :) |
Adding initial serialization methods (readFrom, writeTo) to the PhraseSuggestionBuilder, also adding the base test framework for serialiazation testing, equals and hashCode. Moving SuggestionBuilder out of the global SuggestBuilder for better readability.
cbuescher
force-pushed
the
init-phraseSuggester-refactoring
branch
from
29ca712 to
86db250
Compare
|
Didn't merge this with master but with feature-suggest-refactoring branch in c00c0fa. |
cbuescher
removed
:Search Refactoring
:Search Relevance/Suggesters
Adding initial serialization methods (readFrom, writeTo) to the PhraseSuggestionBuilder, also adding the base test framework for serialiazation testing, equals and hashCode. Moving SuggestionBuilder out of the global SuggestBuilder for better readability.
Still missing: Handling of Smoothing Model (depends on #16130) and DirectCandidateGenerator (depends on #16185). Also Test needs to be extended to handle all parameters.