Add a new query type - ScriptScoreQuery #34533

mayya-sharipova · 2018-10-16T16:23:21Z

script_score query uses script to calculate document scores.
Added as a substitute for function_score with an intention
to deprecate function_score query.

GET /_search
{
    "query": {
        "script_score" : {
            "query": {
                "match": { "message": "elasticsearch" }
            },
            "script" : {
              "source": "_score + Math.log(2 + doc['likes'].value)"
            },
            "min_score" : 2
        }
    }
}

Add several functions to painless to be used inside script_score:

double rational(double, double)
double sigmoid(double, double, double)

double decayGeoLinear(String, String, String, double, GeoPoint)
double decayGeoExp(String, String, String, double, GeoPoint)
double decayGeoGauss(String, String, String, double, GeoPoint)

double decayNumericLinear(double, double, double, double, double)
double decayNumericExp(double, double, double, double, double)
double decayNumericGauss(double, double, double, double, double)

double decayDateLinear(String, String, String, double, JodaCompatibleZonedDateTime)
double decayDateExp(String, String, String, double, JodaCompatibleZonedDateTime)
double decayDateGauss(String, String, String, double, JodaCompatibleZonedDateTime)

Date functions only works on dates in the default format and default time zone

Relates to #23850
Closes #27588, #30303

script_score query uses script to calculate document scores. Added as a substitute for function_score with an intentation to deprecate function_scoreq query. ```http GET /_search { "query": { "script_score" : { "query": { "match": { "message": "elasticsearch" } }, "script" : { "source": "Math.log(2 + doc['likes'].value)" }, "min_score" : 2 } } } ``` Add several functions to painless to be used inside script_score: double rational(double, double) double sigmoid(double, double, double) double decayGeoLinear(String, String, String, double, GeoPoint) double decayGeoExp(String, String, String, double, GeoPoint) double decayGeoGauss(String, String, String, double, GeoPoint) double decayNumericLinear(String, String, String, double, double) double decayNumericExp(String, String, String, double, double) double decayNumericGauss(String, String, String, double, double) double decayDateLinear(String, String, String, double, JodaCompatibleZonedDateTime) double decayDateExp(String, String, String, double, JodaCompatibleZonedDateTime) double decayDateGauss(String, String, String, double, JodaCompatibleZonedDateTime) Date functions only works on dates in the default format and default time zone

elasticmachine · 2018-10-16T16:23:57Z

Pinging @elastic/es-search-aggs

Conky5 · 2018-10-16T19:09:12Z

jenkins test this please

colings86

@mayya-sharipova I left some comments.

colings86 · 2018-10-17T09:56:35Z

docs/reference/query-dsl/script-score-query.asciidoc

+a function to be used to compute a new score for each document returned
+by the query. A script can be written  using either
+<<modules-scripting-painless, painless>> (default)  or
+<<modules-scripting-expression, expression>> languages.


To make this a bit more future proof maybe instead of listing the acceptable languages here it would be better to link to the scripting documentation by saying something like "For more information on scripting see https://www.elastic.co/guide/en/elasticsearch/reference/6.4/modules-scripting.html" ?

colings86 · 2018-10-17T09:58:24Z

docs/reference/query-dsl/script-score-query.asciidoc

+Besides these functions, there are a number of predefined functions
+that can help you with scoring. We suggest you to use them instead of
+rewriting equivalent functions of your own, as these functions try
+to be the most efficient by using the internal painless caching mechanism.


I'm worried about telling users there is caching here when its essentially just an internal optimisation. Maybe instead we can say "these functions try to be the most efficient by using the internal mechanisms."?

colings86 · 2018-10-17T10:00:44Z

docs/reference/query-dsl/script-score-query.asciidoc

+
+* `double decayNumericLinear(String originStr, String scaleStr, String offsetStr, double decay)`
+* `double decayNumericExp(String originStr, String scaleStr, String offsetStr, double decay, double docValue)`
+* `double decayNumericGauss(String originStr, String scaleStr, String offsetStr, double decay, double docValue)`


Why do we use Strings here in the numeric decay functions? Is it not possible to use Number here?

colings86 · 2018-10-17T10:10:58Z

...er/src/main/java/org/elasticsearch/index/query/functionscore/ScriptScoreFunctionBuilder.java

+    public final String getWriteableName() {
+        return getName();
+    }
+


Could you explain why this needed to change? Was there a bug in the script function in the function score query?

colings86 · 2018-10-17T13:06:35Z

server/src/main/java/org/elasticsearch/index/query/functionscore/ScriptScoreQueryBuilder.java

+    @Override
+    protected void doXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.startObject(NAME);
+        if (query != null) {


Is this check necessary? We have a null check for query in the constructor so I don't think this can be null here?

colings86 · 2018-10-17T13:09:16Z

server/src/main/java/org/elasticsearch/index/query/functionscore/ScriptScoreQueryBuilder.java

+        ScriptScoreFunction function = (ScriptScoreFunction) scriptScoreFunctionBuilder.toFunction(context);
+        Query query = this.query.toQuery(context);
+        if (query == null) {
+            query = new MatchAllDocsQuery();


What conditions lead to the toQuery() call emitting null? I am concerned that assuming null can be converted to a MatchAllDocsQuery might be trappy here

colings86 · 2018-10-17T13:13:08Z

server/src/main/java/org/elasticsearch/index/query/functionscore/ScriptScoreQueryBuilder.java

+        String currentFieldName = null;
+        XContentParser.Token token;
+
+        while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {


Is there a reason this parsing can't be done using either ObjectParser or ConstructingObjectParser? I think if we can do it with one of those it would be great as its easier to see what they are doing

mayya-sharipova · 2018-10-17T18:51:48Z

@colings86 Colin, thanks so much for the review, I will address your comments shortly

rjernst · 2018-10-23T05:35:53Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java

                }
            };
            return context.factoryClazz.cast(factory);
+        } else if (context.instanceClazz.equals(ScoreScript.class)) {


I'm confused on why this case is added. I see a couple issues with it:

GenericElasticsearchScript is about to be removed (see Scripting: Remove SearchScript #34730), and all the uses of it (through SearchScript) have already been removed

We already have caching of the compiled script in ScriptService, so why do we need special handling of caching here?

mayya-sharipova · 2018-10-29T12:14:53Z

@colings86 @rjernst I have pushed new commits that address your feedback, can you please continue to review when you have available time

rjernst

This script changes look good to me now. Just a couple small nits.

rjernst · 2018-10-29T19:07:58Z

server/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreQuery.java

+import java.util.Set;
+
+/**
+ * A query that allows for


incomplete javadoc?

rjernst · 2018-10-29T19:10:41Z

server/src/main/java/org/elasticsearch/script/ScoreScript.java

    }

+    /** Return the score of the current document. */
+    public double getScore() {


Why is this added? This would make score available in addition to _score (which is already available with get_score() below). Why do we need the additional score accessor, that deviates from the access in other scripts?

mayya-sharipova · 2018-11-01T11:21:14Z

@colings86 @rjernst I have tried to address your comments. Can you please have another round of review, when you have available time?

colings86

@mayya-sharipova I left a few more comments but I think this is close assuming @rjernst is happy with the changes on the scripting side

colings86 · 2018-11-01T13:10:25Z

server/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreQuery.java

+ */
+public class ScriptScoreQuery extends Query {
+    final Query subQuery;
+    final ScriptScoreFunction function;


Given that the ultimate aim is to remove the Function Score Query entirely it would be great if we could make it so this query doesn't rely on the ScriptScoreFunction either and inlines that logic. I don't think thats completely necessary for this PR but we should consider doing it in a follow up

Yes, you are right. I can address this in the follow-up PR.

colings86 · 2018-11-01T13:19:26Z

server/src/main/java/org/elasticsearch/common/lucene/search/function/ScriptScoreQuery.java

+    public Query rewrite(IndexReader reader) throws IOException {
+        Query rewritten = super.rewrite(reader);
+        if (rewritten != this) {
+            return rewritten;


In most of the Lucene query implementations the call to super.rewrite() is done on the final return line (in this case line 66) so I wonder if its worth doing the same here for readability and consistency? I think the logic would end up being the same we would just be checking the internals of this subclass first?

colings86 · 2018-11-01T13:21:15Z

server/src/main/java/org/elasticsearch/index/query/functionscore/ScriptScoreQueryBuilder.java

+        InnerHitContextBuilder.extractInnerHits(query(), innerHits);
+    }
+
+    private static ConstructingObjectParser<ScriptScoreQueryBuilder, Void> PARSER = new ConstructingObjectParser<>(NAME, false,


nit: Could we put the static blocks, methods and constants at the top of the class?

mayya-sharipova · 2018-11-09T15:06:00Z

@rjernst @colings86 I have tried to address your feedback. Would you have any other comments for this PR?

colings86

My only comment is around random scores and whether we should have a function that caches the instance of Random so the same instance can be used for the whole request across all the documents. Other than that I think this is ready

colings86 · 2018-11-13T13:07:04Z

docs/reference/query-dsl/script-score-query.asciidoc

+[source,js]
+--------------------------------------------------
+"script" : {
+    "source" : "Random rnd = new Random(); rnd.setSeed(doc['field'].value); rnd.nextFloat()"


I think this should use a seed from a parameter rather than from a document?

Also, I assume this uses the parameter caching so the instance of Random is created once and is kept the same through the execution of the script on each doc? If not, then maybe we should do this as it will make searches with the same seed reproducible?

Actually I guess this can't do the caching since we are not using our own function. I think we should implement our own random function that caches the instance of Random with the provided seed?

rjernst

I just have 2 small last questions

rjernst · 2018-11-14T22:06:33Z

server/src/main/java/org/elasticsearch/script/ScoreScript.java

    }

+    /** The leaf lookup for the Lucene segment this script was created for. */
+    protected final LeafSearchLookup getLeafLookup() {


Is this accessed from somewhere? I don't see any uses in this PR.

@rjernst Thanks Ryan, I was copying this from a former SearchScript, this is removed now

rjernst · 2018-11-14T22:09:22Z

server/src/main/java/org/elasticsearch/script/ScoreScript.java

 * A script used for adjusting the score on a per document basis.
 */
-public abstract class ScoreScript {
+public abstract class ScoreScript implements ScorerAware {


What's the reason for needing to add ScorerAware?

@rjernst Thanks Ryan, I was copying this from a former SearchScript, this is removed now

mayya-sharipova · 2018-11-16T02:42:27Z

@colings86 I have added random functions as you suggested, can you please continue to review when you have available time.

colings86

LGTM

rjernst

LGTM

rjernst · 2018-11-20T17:30:38Z

server/src/main/java/org/elasticsearch/script/ScoreScript.java

@@ -1,3 +1,4 @@
+


errant newline?

mayya-sharipova added the :Search/Search Search-related issues that do not fall into other categories label Oct 16, 2018

mayya-sharipova added the >enhancement label Oct 16, 2018

colings86 reviewed Oct 17, 2018

View reviewed changes

rjernst reviewed Oct 23, 2018

View reviewed changes

mayya-sharipova mentioned this pull request Oct 23, 2018

Decay functions should allow to specify a value in case a field is missing #18892

Closed

mayya-sharipova added 3 commits October 25, 2018 14:54

Address Colins' comments

ffe9803

Merge branch 'master' into script-score-query

3eeb5b6

Refactor with new modifications to painless

710365d

rjernst reviewed Oct 29, 2018

View reviewed changes

mayya-sharipova added 2 commits October 31, 2018 10:21

Address Ryan's feedback

f6bfe94

Merge branch 'master' into script-score-query

f9334d7

colings86 reviewed Nov 1, 2018

View reviewed changes

mayya-sharipova added 2 commits November 5, 2018 15:22

Address Colin's comments

0b9cdc0

Merge remote-tracking branch 'upstream/master' into script-score-query

8efbd18

mayya-sharipova force-pushed the script-score-query branch from 4d4e18e to a52e743 Compare November 8, 2018 14:26

Revert back format changes introduced by IDE

2e0b947

mayya-sharipova force-pushed the script-score-query branch from a52e743 to 2e0b947 Compare November 8, 2018 14:28

mayya-sharipova added 2 commits November 8, 2018 15:14

Make script score NOT cacheable in tests

2328368

Merge remote-tracking branch 'upstream/master' into script-score-query

b77038e

This was referenced Nov 9, 2018

Replacing the function_score with discrete queries #27588

Closed

Allow functions to work on unmapped fields #33558

Closed

colings86 reviewed Nov 13, 2018

View reviewed changes

rjernst reviewed Nov 14, 2018

View reviewed changes

mayya-sharipova added 5 commits November 14, 2018 17:39

Add random functions

104372d

Merge branch 'master' into script-score-query

434453b

Use Randomness class instead of java.util.Random

39f7680

Remove unnecessary changes from ScoreScript

4b2f70c

Merge branch 'master' into script-score-query

a16d1d6

colings86 approved these changes Nov 20, 2018

View reviewed changes

rjernst approved these changes Nov 20, 2018

View reviewed changes

mayya-sharipova added 2 commits November 20, 2018 12:44

Remove a new line from ScoreScript.java

36272a7

Merge branch 'master' into script-score-query

d44ec3d

mayya-sharipova merged commit 643bb20 into elastic:master Nov 20, 2018

mayya-sharipova mentioned this pull request Nov 20, 2018

Add functions to the Script context for a new Script Score Query #30303

Closed

14 tasks

jimczi added the v7.0.0 label Nov 22, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

codebrain mentioned this pull request Jun 20, 2019

Implement Script Score Query elastic/elasticsearch-net#3843

Closed

Add a new query type - ScriptScoreQuery #34533

Add a new query type - ScriptScoreQuery #34533

Uh oh!

Conversation

mayya-sharipova commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Oct 16, 2018

Uh oh!

Conky5 commented Oct 16, 2018

Uh oh!

colings86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayya-sharipova commented Oct 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayya-sharipova commented Oct 29, 2018

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayya-sharipova commented Nov 1, 2018

Uh oh!

colings86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayya-sharipova commented Nov 9, 2018

Uh oh!

colings86 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayya-sharipova commented Nov 16, 2018

Uh oh!

colings86 left a comment

Choose a reason for hiding this comment

Uh oh!

mayya-sharipova commented Oct 16, 2018 •

edited

Loading

colings86 left a comment •

edited

Loading