Skip to content

Enable serialization and parsing from xContent for SmoothingModels #16130

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

Merged
merged 2 commits into from
Jan 29, 2016

Conversation

cbuescher
Copy link
Member

PhraseSuggestionBuilder uses three smoothing models internally. In order to enable proper serialization / parsing from xContent to the phrase suggester for the search builder refactoring, this change starts by making the smoothing models writable, adding hashCode/equals and fromXContent.

@cbuescher cbuescher added review :Search Relevance/Suggesters "Did you mean" and suggestions as you type v5.0.0-alpha1 labels Jan 20, 2016
@cbuescher cbuescher force-pushed the refactor-smoothingModel branch from b13a5cb to ce9583c Compare January 21, 2016 10:46
PhraseSuggestionBuilder uses three smoothing models internally.
In order to enable proper serialization / parsing from xContent
to the phrase suggester later, this change starts by making the
smoothing models writable, adding hashCode/equals and fromXContent.
}


public static abstract class SmoothingModel<SM extends SmoothingModel<?>> implements NamedWriteable<SM>, ToXContent {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why we need to add the type parameter to SmoothingModel, I have taken a stab at removing it here. IMO, a non-generic SmoothingModel is simpler. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm absolutely okay to remove it. I think I just added it because we used this for the abstract query builder and some other elements in past refactorings to avoid some casting, but I'm also glad to simlify it here. Will change this.

@cbuescher cbuescher force-pushed the refactor-smoothingModel branch 2 times, most recently from 778c84c to b81be16 Compare January 28, 2016 13:38
@cbuescher
Copy link
Member Author

@areek thanks for the review, I removed the type parameter from SmoothingModel, also added an aditional buildWordScorerFactory() method that we will need later in the Phrase suggesters build method. Can you take another look if this now look okay to you?

@cbuescher cbuescher force-pushed the refactor-smoothingModel branch 2 times, most recently from 0753398 to 920c080 Compare January 28, 2016 13:49
XContentParser parser = parseContext.parser();
XContentParser.Token token;
String fieldName = null;
final double[] lambdas = new double[3];
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: would be nice if these were explicit variables, e.g. trigramLambdaValue, bigramLambdaValue and unigramLambdaValue instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, pure lazyness copied over from current PhraseSuggestParser. Will change that.

@areek
Copy link
Contributor

areek commented Jan 28, 2016

@cbuescher this looks great, LGTM! I added a minor style comment

Adds a method that emits a WordScorerFactory to all of the
three SmoothingModel implementatins that will be needed when
we switch to parsing the PhraseSuggestion on the coordinating
node and need to delay creating the WordScorer on the shards.
@cbuescher cbuescher force-pushed the refactor-smoothingModel branch from 920c080 to aefdee1 Compare January 29, 2016 09:24
cbuescher added a commit that referenced this pull request Jan 29, 2016
Enable Writeable serialization and parsing from xContent for SmoothingModels
@cbuescher cbuescher merged commit b039360 into elastic:master Jan 29, 2016
@cbuescher cbuescher deleted the refactor-smoothingModel branch March 31, 2016 09:26
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Search Refactoring labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Search Relevance/Suggesters "Did you mean" and suggestions as you type :Search/Search Search-related issues that do not fall into other categories v5.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants