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

Similarity should accept dynamic settings when possible #20339

Closed
wants to merge 2 commits into from
Closed

Similarity should accept dynamic settings when possible #20339

wants to merge 2 commits into from

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Sep 6, 2016

This is a spin off for #19046

This change allows to dynamically change options for any existing similarities on an open index.
It uses the affix setting to register all the similarity settings.
It does not allow to change the type of an existing similarity, to change the similarity of a field or to add a new similarity on an open index.
Options are updatable only if they don't change the way the similarity encodes the norm on disk.
The non-updatable options can't be changed if the index is opened nor closed.
This change adds validation to the similarity settings and throws an exception with a detailed message if a setting is unknown or not parseable.
Since it uses affix settings to register non-concrete setting, similarity settings that share the same suffix have been renamed:
lambda has been renamed to collection_model for the IBSimilarity (clashes with the lambda setting in LM Jelinek Mercer similarity).

The downside of using the affix setting is that each setting is registered as a global setting for any similarity type even though some settings are available for a certain similarity type only. Another solution could be to change the way the settings are registered, so instead of:

index.similarity.my_similarity.type: "BM25"
index.similarity.my_similarity.k1: 0.5

... we could do:

index.similarity.my_similarity.BM25.k1: 0.5

This would solve the problem of the global settings but it would also break the settings framework which does not accept settings without value:

index.similarity.my_similarity.BM25

Relates #6727

@jimczi
Copy link
Contributor Author

jimczi commented Sep 6, 2016

@s1monw I am very late on this issue, I was reluctant to do it since this solution has some downsides (listed in the description of the PR) but it should be enough for now especially with the move to BM25 as the default similarity (users may want to tweak the parameters k1 and b).
Could you please take a look when you have time ?

…ilarities on an open index.

It does not allow to change the type of an existing similarity, to change the similarity of a field or to add a new similarity on an open index.
Options are updatable only if they don't change the way the similarity encodes the norm on disk.
The non-updatable options can't be changed if the index is opened nor closed.
This change adds validation to the similarity settings and throws an exception with a detailed message if a setting is unknown or not parseable.
Since it uses affix settings to register non-concrete setting, similarity settings that share the same suffix have been renamed:
`lambda` has been renamed to `collection_model` for the IBSimilarity (clashes with the lambda setting in LM Jelinek Mercer similarity).

Relates #6727
return new NormalizationH2(c);
} else if ("h3".equals(normalization)) {
float c = settings.getAsFloat("normalization.h3.c", 800f);
if (innerSettings.get("h3.c") == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation seems off here?

@s1monw
Copy link
Contributor

s1monw commented Sep 7, 2016

@jimferenczi I took a look and left one concerning comment here #20339 (comment) look good in general. thanks so much for doing this.

@rpedela
Copy link

rpedela commented Oct 15, 2016

Do you have to reindex existing docs for the new parameters to take effect?

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@rjernst
Copy link
Member

rjernst commented Jun 9, 2017

@jimczi @s1monw Where are we on this PR? It at least needs to be updated with master, but I also agree with @s1monw that immutability should be kept.

@jimczi
Copy link
Contributor Author

jimczi commented Jun 9, 2017

Closing since the uber issue is still open and we need a different approach.
Thanks for checking @rjernst

@jimczi jimczi closed this Jun 9, 2017
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Similarities labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Search/Search Search-related issues that do not fall into other categories v6.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants