Skip to content

Enable serialization and parsing from xContent for SmoothingModels#16130

Merged
cbuescher merged 2 commits intoelastic:masterfrom
cbuescher:refactor-smoothingModel
Jan 29, 2016
Merged

Enable serialization and parsing from xContent for SmoothingModels#16130
cbuescher merged 2 commits intoelastic:masterfrom
cbuescher:refactor-smoothingModel

Conversation

@cbuescher
Copy link
Copy Markdown
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.
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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