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

update for ES7.0.0 #219

Merged
merged 5 commits into from
May 3, 2019
Merged

Conversation

umeshdangat
Copy link
Collaborator

@umeshdangat umeshdangat commented Apr 6, 2019

Still some work to be done to get it completely working:

  • Scorer must now implement getMaxScore. This is the upstream lucene PR. For now I return a default value for all Scorer implementations, not sure how this method affects some of the custom Scorers.
  • Got to fix some failing tests in particular
    • lucene does not allow negative scores anymore

@umeshdangat
Copy link
Collaborator Author

Updated to use Elasticsearch 7.0.0
Fixed some tests and relevant code

  • Lucene:7996 - Scorers cant generate negative scores. Removed test which is not relevant anymore and the check for Float.isNan
  • Lucene:8020
    indexSearch.termStatistics can now return null

@umeshdangat umeshdangat changed the title update for ES7.0.0-rc2. Still have a few Todos and some failing tests update for ES7.0.0. Still have a few Todos and some failing tests Apr 10, 2019
@umeshdangat umeshdangat changed the title update for ES7.0.0. Still have a few Todos and some failing tests update for ES7.0.0. Apr 11, 2019
@umeshdangat umeshdangat changed the title update for ES7.0.0. update for ES7.0.0 Apr 11, 2019
@umeshdangat
Copy link
Collaborator Author

Fixed the remaining tests

  • LambdaMART generated negative values, used RankNet instead for those tests. Changed LambaMART output to be positive in one test.
  • Fixed integration tests since es now returns hits.total as an object.

@nomoa @softwaredoug @worleydl I think this PR should be ready for review now.

Copy link
Collaborator

@nomoa nomoa left a comment

Choose a reason for hiding this comment

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

great!

left some comments (mostly minor).

As for the situation about the new restriction of having positive scores I'd be for the following solution while we fix rankers:
Add new param to the ltr/sltr query that would indicate the failure mode in case of negative score returned by some rankers:

  • fail (throw IOException() from the score method)
  • log a warning and clamp min to 0

I'd be for opening a dedicated ticket to discuss this.

src/main/java/com/o19s/es/explore/ExplorerScorer.java Outdated Show resolved Hide resolved
src/main/java/com/o19s/es/explore/ExplorerQuery.java Outdated Show resolved Hide resolved
src/main/java/com/o19s/es/explore/ExplorerQuery.java Outdated Show resolved Hide resolved
src/main/java/com/o19s/es/ltr/query/NoopScorer.java Outdated Show resolved Hide resolved
src/main/java/com/o19s/es/ltr/query/RankerQuery.java Outdated Show resolved Hide resolved
@@ -148,22 +146,19 @@ public void testLogging() throws IOException {
}
}

public void testBogusQuery() throws IOException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should keep this test perhaps just expect that IllegalArgumentException is thrown?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The issue is we cant even build a BoostQuery object anymore since that is the one that throws IllegalArgumentException so essentially we never even get to the code we want to test.

@nomoa
Copy link
Collaborator

nomoa commented Apr 18, 2019

and the circleci config will have to be updated to pull java 12 I suppose.

@jettro
Copy link
Collaborator

jettro commented Apr 22, 2019

Curious about the status of the PR. I have used the plugin with my basic sample, everything still seems to work.

@umeshdangat
Copy link
Collaborator Author

@jettro I need to address the comments mentioned by @nomoa. Should get to them sometime later this week. Been somewhat busy with https://haystackconf.com/2019/evolution/.
In case anyone of you want to go ahead and address some issues please feel free to go ahead or I will get to them most likely post haystack.

@umeshdangat
Copy link
Collaborator Author

and the circleci config will have to be updated to pull java 12 I suppose.

I dont see openjdk-12 image available on circleci. https://circleci.com/docs/2.0/circleci-images/#openjdk

Copy link
Collaborator

@nomoa nomoa left a comment

Choose a reason for hiding this comment

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

LGTM!

@softwaredoug softwaredoug merged commit 8e370a2 into o19s:master May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants