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

Add minimal sanity checks to custom/scripted similarities. (backport) #33893

Merged
merged 1 commit into from
Sep 25, 2018

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Sep 20, 2018

Add minimal sanity checks to custom/scripted similarities.

Lucene 8 introduced more constraints on similarities, in particular:

  • scores must not be negative,
  • scores must not decrease when term freq increases,
  • scores must not increase when norm (interpreted as an unsigned long)
    increases.

We can't check every single case, but could at least run some sanity checks.

Backport of #33564

Add minimal sanity checks to custom/scripted similarities.

Lucene 8 introduced more constraints on similarities, in particular:
 - scores must not be negative,
 - scores must not decrease when term freq increases,
 - scores must not increase when norm (interpreted as an unsigned long)
   increases.

We can't check every single case, but could at least run some sanity checks.

Backport of elastic#33564
@jpountz jpountz added >enhancement :Search Relevance/Ranking Scoring, rescoring, rank evaluation. v6.5.0 labels Sep 20, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

I let one comment, which doesn't really need to be acted on. Otherwise LGTM

}

private static class SingleNormLeafReader extends LeafReader {

Copy link
Contributor

Choose a reason for hiding this comment

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

I do wonder if this class should be in a separate utility file somewhere? It's a third of the entire file length. On the other hand, this is all going away in master, so maybe it doesn't matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll leave things this way though as master doesn't have the issue so it's not a big deal like you said.

@jpountz jpountz merged commit a957866 into elastic:6.x Sep 25, 2018
@jpountz
Copy link
Contributor Author

jpountz commented Sep 25, 2018

Thanks @jimczi and @romseygeek

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search Relevance/Ranking Scoring, rescoring, rank evaluation. v6.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants