SCRIPTING: Move sort Context to its Own Class by original-brownbear · Pull Request #33717 · elastic/elasticsearch

original-brownbear · 2018-09-14T13:03:25Z

SCRIPTING: Move sort Context to its Own Class
Extracted SortScript
Kept mechanics close to what they were with SearchScript
- execute appears to return String as well as Number
  in certain scenarios so I had to keep the Object return here

* SCRIPTING: Move sort Context to its Own Class * Extracted SortScript * Kept mechanics close to what they were with SearchScript * `execute` appears to return `String` as well as `Number` in certain scenarios so I had to keep the `Object` return here

elasticmachine · 2018-09-14T13:03:26Z

Pinging @elastic/es-core-infra

…t-context

rjernst

I wonder if we could have 2 different script classes for String/Number, so we can have type safety? The compilation of the sort script happens at the beginning of ScriptSortBuilder.build, but I think it could be moved inside the switch on type, so the factories/script classes are specific to each type.

original-brownbear · 2018-09-14T17:16:00Z

@rjernst sure, we can do that :) Will do soon.

…t-context

original-brownbear · 2018-09-17T09:51:35Z

@rjernst done, split the sort context up into 2 contexts for strings and numbers now :)

rjernst

I left a few comments. Don't expressions need to be updated to support the numeric version?

rjernst · 2018-09-17T18:42:43Z

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

+        return execute().doubleValue();
+    }
+
+    public abstract Number execute();


Why does this need to return number? We only ever seem to convert it to double, so I think it should return that?

@rjernst 👍 jup

rjernst · 2018-09-17T18:43:17Z

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

+        super(params, lookup, leafContext);
+    }
+
+    public double runAsDouble() {


I don't think we need this, the callers should just call execute().

Right, my bad didn't double check after splitting this => removing :)

rjernst · 2018-09-17T18:44:05Z

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

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


This will cause a variable score to be made available to the script, not _score. It needs to be get_score().

right 👍 renaming :)

rjernst · 2018-09-17T18:45:59Z

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

+import org.elasticsearch.search.lookup.LeafSearchLookup;
+import org.elasticsearch.search.lookup.SearchLookup;
+
+abstract class AbstractSortScript implements ScorerAware {


I don't think this can be package private, as painless would not be able to access the methods from it?

I'm also not sure there is really enough here to justify a super class. Can we just have the few lines duplicated and abstract this when we see more need?

@rjernst I mainly made the abstract class because I figured there's quite a bit of magic with the way the lookups work and such and it wasn't so tricky to make this.

Also, it's fine for this to be package private. Painless uses Class#getMethods() which is documented as:

* Returns an array containing {@code Method} objects reflecting all the * public methods of the class or interface represented by this {@code * Class} object, including those declared by the class or interface and * those inherited from superclasses and superinterfaces.

Thanks, that is good to know about the lookups, I didn't realize that. :)

I would at least rename this as I don't think it has to do with "sorting" at all, it is about making score accessible?

@rjernst true, AbstractScoreAwareScript ? :)

original-brownbear · 2018-09-17T19:07:07Z

@rjernst

Don't expressions need to be updated to support the numeric version?

Now that you mention it ... didn't we already forget this in #33602 as well? (we needed to add logic to ExpressionScriptEngine to support the new context there too?)
... also we should probably add some integration tests for expressions scripts + the contexts they support

…t-context

original-brownbear · 2018-10-16T19:20:49Z

just FYI, I'm back to fixing this now the question above has answered itself in the other PRs :)

…t-context

original-brownbear · 2018-10-16T22:20:49Z

@rjernst this should be good for another review. I made it work for expressions now for numeric sorts :)

rjernst

Thanks! LGTM, just a couple minor nits

rjernst · 2018-10-17T00:25:34Z

...ang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java

+                if (variable.equals("_score")) {
+                    bindings.add(new SortField("_score", SortField.Type.SCORE));
+                    needsScores = true;
+                } else if (variable.equals("_value")) {


_value is only for agg scripts, we shouldn't have it for anything else

rjernst · 2018-10-17T00:28:03Z

test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java

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


missing "else" here for the else if?

original-brownbear · 2018-10-17T09:02:22Z

@rjernst thanks!

* SCRIPTING: Move sort Context to its own Class

original-brownbear added >non-issue :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v7.0.0 v6.5.0 labels Sep 14, 2018

original-brownbear requested a review from rjernst September 14, 2018 13:03

Merge remote-tracking branch 'elastic/master' into extract-sort-scrip…

38bcbfe

…t-context

rjernst reviewed Sep 14, 2018

View reviewed changes

original-brownbear added 3 commits September 17, 2018 10:06

Merge remote-tracking branch 'elastic/master' into extract-sort-scrip…

0c95cc2

…t-context

CR: Split Sort into 2 contexts for the two types

5ffebc5

drier

96e109c

original-brownbear added 2 commits September 17, 2018 15:30

Merge remote-tracking branch 'elastic' into extract-sort-script-context

2a3bb97

fix test

61bbabe

rjernst reviewed Sep 17, 2018

View reviewed changes

original-brownbear added 2 commits September 17, 2018 21:08

CR: Fix _score accessor + remove redundant executeAsDouble

8b72cfe

Merge remote-tracking branch 'elastic/master' into extract-sort-scrip…

5dbf6a0

…t-context

original-brownbear mentioned this pull request Sep 19, 2018

SCRIPTING: Terms set query expression #33856

Merged

original-brownbear added 2 commits October 16, 2018 21:33

Merge remote-tracking branch 'elastic/master' into extract-sort-scrip…

7527f8c

…t-context

CR: Enable expressions for numeric sort scripts

75fc3c8

rjernst approved these changes Oct 17, 2018

View reviewed changes

original-brownbear added 2 commits October 17, 2018 07:49

Merge remote-tracking branch 'elastic' into extract-sort-script-context

0e07597

CR: remove _value from NumberSortScript and add missing elase

94b252f

original-brownbear merged commit 3954d04 into elastic:master Oct 17, 2018

original-brownbear deleted the extract-sort-script-context branch October 17, 2018 09:02

kcm pushed a commit that referenced this pull request Oct 30, 2018

SCRIPTING: Move sort Context to its Own Class (#33717)

47af231

* SCRIPTING: Move sort Context to its own Class

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

Conversation

original-brownbear commented Sep 14, 2018

Uh oh!

elasticmachine commented Sep 14, 2018

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Sep 14, 2018

Uh oh!

original-brownbear commented Sep 17, 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!

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!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

original-brownbear commented Oct 16, 2018

Uh oh!

original-brownbear commented Oct 16, 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!

original-brownbear commented Oct 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

original-brownbear commented Sep 17, 2018 •

edited

Loading