Add script engine for Lucene expressions #6819

rjernst · 2014-07-10T18:56:45Z

Adds support for new "expression" script engine.

This is backed by lucene expressions. These are javascript expressions, which can only access numeric fielddata, parameters, and _score. They can only be used for searches (not document updates).

closes #6818

(MapperService causing errors).

…iven field name).

rmuir · 2014-07-10T19:08:38Z

src/main/java/org/elasticsearch/script/expression/ExpressionScript.java

+        scorer = new CannedScorer();
+    }
+
+    double evaluate() {


Can we remove this expensive per-document stuff? It sorta totally defeats the point of having expressions!

Can these hacks be moved to setScorer or setNextReader? why isn't setScorer being called, can you expand on the comment? that seems broken...

Yeah this is a goof. I did this just to get it working, and then forgot to fix it! Of course tests pass, but I'm still working on benchmarking so I haven't seen the slowness yet.

I'll expand the comment in setScorer, and will come up with a hack to allow setNextVar to work through a single FunctionValues. It will still be a little slower than normal expressions for the aggregations case because it will do a hash of the variable name.

rmuir · 2014-07-10T19:13:02Z

Hi ryan, I left an initial comment. Is there anything we can do to remove the expensive per-document stuff? I realize these might be limitations of the scripting API, but maybe we can fix that? Otherwise, if we are doing things like hashing strings etc per-document to resolve variable names, its going to be very slow.

s1monw · 2014-07-10T19:58:55Z

pom.xml

@@ -150,6 +150,12 @@
            <scope>compile</scope>
        </dependency>
        <dependency>
+            <groupId>org.apache.lucene</groupId>
+            <artifactId>lucene-expressions</artifactId>
+            <version>${lucene.version}</version>


I am not sure if we should mark it optional due to all the dependencies it has maybe @kimchy knows what to do?

It only has two: antlr-runtime and asm. I don't think its that many :)

I agree with @rmuir. If we make it optional, we would have to do some magic to make the expression script engine load only if expressions are available (which I guess isn't that hard to do, but the other built in script engines don't assume they are optional).

It should be optional, but we should make sure we package it as part of our distro. In our script service we already support optional mvel and groovy, so thats easy, similar logic.

Its less about the size, its about the fact that those are 2 very popular libraries, and almost certainly will cause clashes when someone wants to use a NodeClient / TransportClient

In order to include it in our distro, make sure to add it in the deb/rpm logic in the pom.xml (with all deps), and in src/main/assemblies/common-bin, then run the package and check tar.gz includes all the deps, as well as the deb and rpm

Ok I made the dep optional. I didn't need to change anything else since the lib starts with "lucene" and lucene* is already included. I checked the tar.gz and it includes all 3 deps.

s1monw · 2014-07-10T20:09:42Z

I left some initial comments.. I will remove the review label for now feel free to put it back once you have new stuff to review

rjernst · 2014-07-10T21:25:24Z

@rmuir @s1monw I think I addressed all of your comments.

rmuir · 2014-07-10T21:37:10Z

src/main/java/org/elasticsearch/script/expression/ExpressionScript.java

+
+    @Override
+    public void setNextScore(float score) { scorer.score = score; }
+


Can we add another TODO here? This seems massively inefficient:

expressions know if they refer to score, true or false. in lucene, Scorer.score() will only be called, if the expression actually needs it.

here it seems, something calls score() always, and passes it to the expression even if its maybe not needed? so it might be called uselessly on the scorer, even if the ranking function doesnt use it.

passing score in this hacky way im sure prevents inlining of actual scoring function into the expression and hurts performance.

Ok added it.

…ess documentation).

…ve and groovy scripts

rjernst · 2014-07-12T00:19:46Z

Ok, I have some benchmark numbers. The following tests were done with 1 million random docs, and 1k iterations of a match_all query using a script for sorting. The script is described in each column and each cell is the average time per request.

lang	x	x + y	1.2 * x / y	sqrt(abs(z)) + ln(abs(x * y))
expression	5 ms	14 ms	14 ms	26 ms
native	14 ms	22 ms	23 ms	38 ms
groovy	37 ms	71 ms	74 ms	116 ms

clintongormley · 2014-07-12T10:25:16Z

awesome numbers! expressions will be a very welcome addition to scripting

s1monw · 2014-07-14T19:20:18Z

pom.xml

@@ -873,7 +880,7 @@
                                </data>
                                <data>
                                    <src>${project.build.directory}/lib</src>
-                                    <includes>lucene*, log4j*, jna*, spatial4j*, jts*, groovy*</includes>
+                                    <includes>lucene*, log4j*, jna*, spatial4j*, jts*, groovy*, </includes>


is this extra , needed?

Not sure how that got there. Removing.

s1monw · 2014-07-14T19:27:34Z

I left some cosmetic comments. LGTM

rjernst · 2014-07-14T21:10:20Z

Ok, I think I addressed all your issues. I will fix setScorer in another PR.

rjernst · 2014-07-14T22:22:12Z

I implemented a partial fix for setScorer in #6864.

s1monw · 2014-07-15T11:22:21Z

LGTM I think we should get this into 1.3 I will tag it as that so feel free to push it there

uschindler · 2014-07-20T12:01:55Z

Maybe open another issue about this:
The Expressions module is safe to use for anybody, because you cannot invoke any random java functions (just what is provided via the bindings and the Map of Double-Methods.
Currently you can only disable scripting completely in ES (if you want to make search APIs public to internet via reverse proxy). It would be cool to make the script engines that are available via REST configureable.
Should I open an issue or is this already solved here (I have not yet looked closely into this patch).

clintongormley · 2014-07-20T12:13:27Z

@uschindler from 1.3, sandboxed languages (ie groovy and expressions) are enabled by default: http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/modules-scripting.html#_enabling_dynamic_scripting

uschindler · 2014-07-20T12:15:03Z

Thanks @clintongormley !

rjernst added 9 commits July 2, 2014 09:45

Start of expressions integration as a script engine. Does not run

f61f23f

(MapperService causing errors).

Basic contant parameters and field access (through variables of the g…

a95c289

…iven field name).

Fixed _score to actually work

dfae59b

Added copies changes from LUCENE-5806. Finished tests.

892d980

Fixed possible NPE when mapping doesn't exist and added test

c3726a8

Merge branch 'master' into feature/expressions

58eb558

Add assert to trip on upgrade to 4.10

b8c8644

Test terms aggs fail with expressions

631a38d

Minor readability tweaks before PR

8edb0ce

rjernst added v1.3.0 labels Jul 10, 2014

rmuir reviewed Jul 10, 2014
View reviewed changes

Merge branch 'master' into feature/expressions

ad96e83

s1monw reviewed Jul 10, 2014
View reviewed changes

s1monw removed the review label Jul 10, 2014

Address first round of review comments

8a31ae1

rjernst added the review label Jul 10, 2014

rmuir reviewed Jul 10, 2014
View reviewed changes

rjernst added 2 commits July 11, 2014 11:23

Change expression disk extension to match langauge name (to require l…

93f8bb3

…ess documentation).

Tweaked documentation and add benchmark comparing expressions to nati…

ec49e96

…ve and groovy scripts

s1monw added v1.4.0 and removed v1.3.0 labels Jul 14, 2014

rjernst added 2 commits July 14, 2014 07:20

Adding experimental warning to docs

1a8154b

Merge branch 'master' into feature/expressions

737b1c3

s1monw reviewed Jul 14, 2014
View reviewed changes

s1monw removed the review label Jul 14, 2014

Address more review comments

140f7da

s1monw added v1.3.0 and removed v1.4.0 labels Jul 15, 2014

rjernst closed this Jul 15, 2014

clintongormley changed the title ~~Scripting: Add script engine for lucene expressions~~ Scripting: Add script engine for Lucene expressions Jul 16, 2014

clintongormley added the release highlight label Jul 16, 2014

rjernst deleted the feature/expressions branch January 21, 2015 23:22

clintongormley added the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Jun 6, 2015

clintongormley changed the title ~~Scripting: Add script engine for Lucene expressions~~ Add script engine for Lucene expressions Jun 6, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add script engine for Lucene expressions #6819

Add script engine for Lucene expressions #6819

rjernst commented Jul 10, 2014

rmuir Jul 10, 2014

rjernst Jul 10, 2014

rmuir commented Jul 10, 2014

s1monw Jul 10, 2014

rmuir Jul 10, 2014

rjernst Jul 10, 2014

kimchy Jul 10, 2014

rjernst Jul 10, 2014

s1monw commented Jul 10, 2014

rjernst commented Jul 10, 2014

rmuir Jul 10, 2014

rjernst Jul 10, 2014

rjernst commented Jul 12, 2014

clintongormley commented Jul 12, 2014

s1monw Jul 14, 2014

rjernst Jul 14, 2014

s1monw commented Jul 14, 2014

rjernst commented Jul 14, 2014

rjernst commented Jul 14, 2014

s1monw commented Jul 15, 2014

uschindler commented Jul 20, 2014

clintongormley commented Jul 20, 2014

uschindler commented Jul 20, 2014


		@Override
		public void setNextScore(float score) { scorer.score = score; }

Add script engine for Lucene expressions #6819

Add script engine for Lucene expressions #6819

Conversation

rjernst commented Jul 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmuir commented Jul 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1monw commented Jul 10, 2014

rjernst commented Jul 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Jul 12, 2014

clintongormley commented Jul 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1monw commented Jul 14, 2014

rjernst commented Jul 14, 2014

rjernst commented Jul 14, 2014

s1monw commented Jul 15, 2014

uschindler commented Jul 20, 2014

clintongormley commented Jul 20, 2014

uschindler commented Jul 20, 2014