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

Script with `_score`: remove dependency of DocLookup and scorer #7819

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@brwe
Copy link
Contributor

brwe commented Sep 22, 2014

As pointed out in #7487 DocLookup is a variable that is accessible by all scripts
for one doc while the query is executed. But the _score and therfore the scorer
depends on the current context, that is, which part of query is currently executed.
Instead of setting the scorer for DocLookup
and have Script access the DocLookup for getting the score, the Scorer should just
be explicitely set for each script.
DocLookup should not have any reference to a scorer.
This was similarly discussed in #7043.

This dependency caused a stackoverflow when running script score in combination with an
aggregation on _score. Also the wrong scorer was called when nesting several script scores.

closes #7487

script with _score: remove dependency of DocLookup and scorer
As pointed out in #7487 DocLookup is a variable that is accessible by all scripts
for one doc while the query is executed. But the _score and therfore the scorer
depends on the current context, that is, which part of query is currently executed.
Instead of setting the scorer for DocLookup
and have Script access the DocLookup for getting the score, the Scorer should just
be explicitely set for each script.
DocLookup should not have any reference to a scorer.
This was similarly discussed in #7043.

This dependency caused a stackoverflow when running script score in combination with an
aggregation on _score. Also the wrong scorer was called when nesting several script scores.

closes #7487

@brwe brwe added the review label Sep 22, 2014

@dadoonet

This comment has been minimized.

Copy link
Member

dadoonet commented Sep 22, 2014

Just to keep a link of it, adding a comment that this change could help fixing:

@brwe

This comment has been minimized.

Copy link
Contributor Author

brwe commented Sep 22, 2014

@colings86 @dadoonet I am wondering what to do with 1.3. It is not really broken there, just inconvenient: To use _score in aggregations and sorting, one has to use doc.score but when we use it in a script score it must be accessed with _score. Can we just leave it this way for 1.3.x? Wondering because of #7712 and #7752

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Sep 25, 2014

LGTM

@s1monw s1monw removed the review label Sep 25, 2014

@brwe brwe closed this in 7feb742 Sep 26, 2014

brwe added a commit that referenced this pull request Sep 26, 2014

script with _score: remove dependency of DocLookup and scorer
As pointed out in #7487 DocLookup is a variable that is accessible by all scripts
for one doc while the query is executed. But the _score and therfore the scorer
depends on the current context, that is, which part of query is currently executed.
Instead of setting the scorer for DocLookup
and have Script access the DocLookup for getting the score, the Scorer should just
be explicitely set for each script.
DocLookup should not have any reference to a scorer.
This was similarly discussed in #7043.

This dependency caused a stackoverflow when running script score in combination with an
aggregation on _score. Also the wrong scorer was called when nesting several script scores.

closes #7487
closes #7819

brwe added a commit that referenced this pull request Sep 26, 2014

script with _score: remove dependency of DocLookup and scorer
As pointed out in #7487 DocLookup is a variable that is accessible by all scripts
for one doc while the query is executed. But the _score and therfore the scorer
depends on the current context, that is, which part of query is currently executed.
Instead of setting the scorer for DocLookup
and have Script access the DocLookup for getting the score, the Scorer should just
be explicitely set for each script.
DocLookup should not have any reference to a scorer.
This was similarly discussed in #7043.

This dependency caused a stackoverflow when running script score in combination with an
aggregation on _score. Also the wrong scorer was called when nesting several script scores.

closes #7487
closes #7819

@clintongormley clintongormley changed the title script with _score: remove dependency of DocLookup and scorer Scripting: Script with _score: remove dependency of DocLookup and scorer Sep 26, 2014

brwe added a commit to brwe/elasticsearch-lang-python that referenced this pull request Sep 30, 2014

brwe added a commit to brwe/elasticsearch-lang-python that referenced this pull request Sep 30, 2014

@brwe brwe added the >breaking label Oct 1, 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)

@clintongormley clintongormley changed the title Scripting: Script with _score: remove dependency of DocLookup and scorer Script with `_score`: remove dependency of DocLookup and scorer Jun 6, 2015

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

script with _score: remove dependency of DocLookup and scorer
As pointed out in elastic#7487 DocLookup is a variable that is accessible by all scripts
for one doc while the query is executed. But the _score and therfore the scorer
depends on the current context, that is, which part of query is currently executed.
Instead of setting the scorer for DocLookup
and have Script access the DocLookup for getting the score, the Scorer should just
be explicitely set for each script.
DocLookup should not have any reference to a scorer.
This was similarly discussed in elastic#7043.

This dependency caused a stackoverflow when running script score in combination with an
aggregation on _score. Also the wrong scorer was called when nesting several script scores.

closes elastic#7487
closes elastic#7819
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.