Upgrade to Lucene 6.4.0#22724
Conversation
|
LGTM, except I'm not so familiar with the |
|
Thanks @mikemccand . I pushed some modifications to fix the |
jdconrad
left a comment
There was a problem hiding this comment.
LGTM! My only concern is if any other ValueSource uses score the needsScore will be ignored by expressions in ES now (but at this time I don't think this is a problem.) It's possible I'm misreading this, though.
You're not, we need to check if the value source needs the score as well. My last commit fixes this. The build passes for me locally, I'll push when the ci finishes. Thanks for the review @jdconrad ! |
* Upgrade to Lucene 6.4.0 `ValueSource`s are now converted to `DoubleValueSource`s using the Lucene adapter made for the migration to the new API in 6.4.0.
* Upgrade to Lucene 6.4.0 `ValueSource`s are now converted to `DoubleValueSource`s using the Lucene adapter made for the migration to the new API in 6.4.0.
| public boolean advanceExact(int doc) throws IOException { | ||
| return true; | ||
| } | ||
| }); |
There was a problem hiding this comment.
you could reuse DoubleValuesSource.fromScorer(scorer) here
There was a problem hiding this comment.
No we can't ;).
There is an assert in the scorer created in this function that checks that the scorer's docid is set when advanceExact is called. Though the canned scorer that we use set the docid when score is called which makes it incompatible with this helper.
| DoubleValues values = source.getValues(leaf, new DoubleValues() { | ||
| @Override | ||
| public double doubleValue() throws IOException { | ||
| return Double.NaN; |
There was a problem hiding this comment.
would it be less trappy to throw an exception instead like we did before?
There was a problem hiding this comment.
Same here, we can't do that. The correct fix would be to pass a null value but it's also not possible currently because some DoubleValueSources that need to access the score check that the scorer is not null when getValues is called. We could also check if the source needs the score but this information is lost in the ValueSource to DoubleValueSource conversion because the converted DoubleValueSource always returns true when needsScore is invoked.
|
Thanks @jpountz . I think there are some things to clean up in this PR but it will require some changes on our side to be able to do it. This is why I choose to push this PR as is for 5.2 but we definitely need to review this for 5.x and master. |
* master: (33 commits) Docs fix - Added missing link to new Adjacency-matrix agg Pass `forceExecution` flag to transport interceptor (elastic#22739) Version: Add missing releases from 2.x in Version.java (elastic#22594) CONSOLE-ify filter aggregation docs CONSOLE-ify date_range aggregation docs Add single static instance of SpecialPermission (elastic#22726) Simplify InternalEngine#innerIndex (elastic#22721) Upgrade to Lucene 6.4.0 (elastic#22724) Fix broken TaskInfo.toString() Add CheckedSupplier and CheckedRunnable to core (elastic#22725) Revert "Make build Gradle 2.14 / 3.x compatible (elastic#22669)" Fixes retrieval of the latest snapshot index blob (elastic#22700) CONSOLE-ify date histogram docs CONSOLE-ify min and max aggregation docs CONSOLE-ify global-aggregation.asciidoc Fix script score function that combines _score and weight (elastic#22713) Corrected a plural verb to a singular one. (elastic#22681) Fix duplicates from search.query (elastic#22701) Readd unconverted snippets mark for doc Deguice rest handlers (elastic#22575) ...
This change updates the dependencies to Lucene 6.4.0 and fixes the compilation errors due to some changes in Lucene API.