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

Remove setNextScore in SearchScript. #6864

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@rjernst
Copy link
Member

commented Jul 14, 2014

While it would be nice to do this all the way up the chain (into
score functions), this at least removes the weird dual
setNextScore/setScorer for SearchScripts.

@s1monw

View changes

src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreFunction.java Outdated
import java.util.Map;

public class ScriptScoreFunction extends ScoreFunction {

static class CannedScorer extends Scorer {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 15, 2014

Contributor

can this class be final?

This comment has been minimized.

Copy link
@rjernst

rjernst Jul 15, 2014

Author Member

Yep, done.

@@ -37,8 +37,6 @@

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 15, 2014

Contributor

I think we should also get rid of setNextDocId() since it's part of the scorer by definition? if you wanna make this work in a second PR that's fine too

This comment has been minimized.

Copy link
@rjernst

rjernst Jul 15, 2014

Author Member

We can't do that. The scorer is only used for when we are scoring. But setNextDocId() is used to advance the doc we are on regardless of whether we use score or not.

this.value = value;
float score() {
try {
return lookup.doc().score();

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 15, 2014

Contributor

can't we access the scorer directly? do we have to do the map lookup?

This comment has been minimized.

Copy link
@rjernst

rjernst Jul 15, 2014

Author Member

What do you mean by the "map" lookup? SearchLookup handles maintaining the scorer (since it is ScorerAware). I'm sure this can be simplified in the future, but this PR was just to start by getting rid of setNextScore.

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2014

I left some comments... look good in general

@s1monw s1monw removed the review label Jul 15, 2014

@kimchy

This comment has been minimized.

Copy link
Member

commented Jul 16, 2014

LGTM, we should make sure we fix mvel as well, not just groovy

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2014

@kimchy Mvel it removed in master that is why it's not in the PR it's still on 1.x so I guess ryan is fixing it once he is porting the commit

Remove setNextScore in SearchScript.
While it would be nice to do this all the way up the chain (into
score functions), this at least removes the weird dual
setNextScore/setScorer for SearchScripts.
@rjernst

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2014

Yes, I will fix mvel in 1.x when I backport to there.

@rjernst rjernst closed this in 770447c Jul 16, 2014

rjernst added a commit that referenced this pull request Jul 16, 2014

Scripting: Remove setNextScore in SearchScript.
While it would be nice to do this all the way up the chain (into
score functions), this at least removes the weird dual
setNextScore/setScorer for SearchScripts.

closes #6864

rjernst added a commit to rjernst/elasticsearch that referenced this pull request Jul 16, 2014

Scripts: Make ScoreAccessor utility class plublicly available for oth…
…er script

engines to use.

With the removal of setNextScore in elastic#6864, script engines must use
the Scorer to find the score of a document.  The DocLookup is updated
appropriately to do this, but most script engines require a Number to be
bound for numeric variables.  Groovy already had an encapsulation for
this funtionality, and this moves it out to be shared with other script
engines.

rjernst added a commit that referenced this pull request Jul 16, 2014

Scripts: Make ScoreAccessor utility available for other script engines.
With the removal of setNextScore in #6864, script engines must use
the Scorer to find the score of a document.  The DocLookup is updated
appropriately to do this, but most script engines require a Number to be
bound for numeric variables.  Groovy already had an encapsulation for
this funtionality, and this moves it out to be shared with other script
engines.

closes #6898

rjernst added a commit that referenced this pull request Jul 17, 2014

Scripts: Make ScoreAccessor utility available for other script engines.
With the removal of setNextScore in #6864, script engines must use
the Scorer to find the score of a document.  The DocLookup is updated
appropriately to do this, but most script engines require a Number to be
bound for numeric variables.  Groovy already had an encapsulation for
this funtionality, and this moves it out to be shared with other script
engines.

closes #6898

@clintongormley clintongormley changed the title Remove setNextScore in SearchScript. Scripting: Remove setNextScore in SearchScript. Sep 8, 2014

brwe added a commit to elastic/elasticsearch-lang-javascript that referenced this pull request Oct 7, 2014

Remove setNextScore in SearchScript
Due to a change in elasticsearch 1.4.0, we need to apply a similar patch here.

See elastic/elasticsearch#6864
See elastic/elasticsearch#7819

Closes #23.
(cherry picked from commit 5e5c373)

brwe added a commit to elastic/elasticsearch-lang-javascript that referenced this pull request Oct 7, 2014

Remove setNextScore in SearchScript
Due to a change in elasticsearch 1.4.0, we need to apply a similar patch here.

See elastic/elasticsearch#6864
See elastic/elasticsearch#7819

Closes #23.

brwe added a commit to elastic/elasticsearch-lang-javascript that referenced this pull request Oct 7, 2014

Remove setNextScore in SearchScript
Due to a change in elasticsearch 1.4.0, we need to apply a similar patch here.

See elastic/elasticsearch#6864
See elastic/elasticsearch#7819

Closes #23.
(cherry picked from commit 5e5c373)

brwe added a commit to elastic/elasticsearch-lang-python that referenced this pull request Oct 7, 2014

Remove setNextScore in SearchScript
Due to a change in elasticsearch 1.4.0, we need to apply a similar patch here.

See elastic/elasticsearch#6864
See elastic/elasticsearch#7819

Closes #16.
Closes #21.

brwe added a commit to elastic/elasticsearch-lang-python that referenced this pull request Oct 7, 2014

Remove setNextScore in SearchScript
Due to a change in elasticsearch 1.4.0, we need to apply a similar patch here.

See elastic/elasticsearch#6864
See elastic/elasticsearch#7819

Closes #16.
Closes #21.

(cherry picked from commit cd7756c)

brwe added a commit to elastic/elasticsearch-lang-python that referenced this pull request Oct 7, 2014

Remove setNextScore in SearchScript
Due to a change in elasticsearch 1.4.0, we need to apply a similar patch here.

See elastic/elasticsearch#6864
See elastic/elasticsearch#7819

Closes #16.
Closes #21.

(cherry picked from commit cd7756c)

@rjernst rjernst deleted the rjernst:fix/script-scorer branch Jan 21, 2015

@clintongormley clintongormley changed the title Scripting: Remove setNextScore in SearchScript. Remove setNextScore in SearchScript. Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.