Convert double script to return array by nik9000 · Pull Request #61504 · elastic/elasticsearch

nik9000 · 2020-08-24T20:14:54Z

This replaces the value collection method in double valued runtime
script fields with simply returning a double[]. Painless has some
"convert" features that allow us to define automatic conversions from
things like double and Collection into double[] so we use those.

Relates to #59332

elasticmachine · 2020-08-24T20:14:56Z

Pinging @elastic/es-search (:Search/Search)

This replaces the value collection method in `double` valued runtime script fields with simply returning a `double[]`. Painless has some "convert" features that allow us to define automatic conversions from things like `double` and `Collection` into `double[]` so we use those.

nik9000 · 2020-08-24T20:15:29Z

...mlRestTest/java/org/elasticsearch/xpack/runtimefields/rest/CoreTestsWithRuntimeFieldsIT.java

            return null;
        }
        StringBuilder b = new StringBuilder();
+        if ("double".equals(type)) {


The if is temporary and it'll become all the time once it is done.

nik9000 · 2020-08-24T20:16:05Z

...ields/src/main/java/org/elasticsearch/xpack/runtimefields/AbstractLongScriptFieldScript.java

        super(params, searchLookup, ctx);
    }

+    public abstract void execute();


Because I'm only doing one of them I have to move the old execute method decalarations into the subclasses.

nik9000 · 2020-08-24T20:17:10Z

...rc/main/java/org/elasticsearch/xpack/runtimefields/query/AbstractDoubleScriptFieldQuery.java

     * Does the value match this query?
     */
-    protected abstract boolean matches(double[] values, int count);
+    protected abstract boolean matches(double[] values);


We don't need the count any more because we use the whole array.

javanna

left a couple of questions. Shall we have unit tests for the static conversion methods? LGTM otherwise.

javanna · 2020-08-25T13:49:12Z

...time-fields/src/main/java/org/elasticsearch/xpack/runtimefields/DoubleScriptFieldScript.java

+        if (o instanceof Double) {
+            return convertFromDouble(((Double) o).doubleValue());
+        } else {
+            return (double[]) o;


oh boy I was hoping we would not need this sort of stuff, but I guess we do? I mean the instanceof as well as the cast to double array

😢 Me too! I imagine the painless folk are thinking about it, but I'm not sure. This seems like a perfect spot for invokedynamic, but I'm not an expert at all.

what happens if we do without this conversion? Really bad I guess?

@javanna without it you can't really return def. You get a lot of funny class cast exceptions.

@stu-elastic or @jdconrad do we have plans indy-ify this or something? So it'd call the converter based on the def type. Is that what #61389 is all about?

We needed to get this in your hands ASAP. We'll be improving it: #61389

Indy for this is really challenging. We are going to look into it, but allowing you to do def conversions this way ensures we have something for runtime fields now that doesn't use reflection invocation. I would recommend that this cover all numeric cases including byte through long as well. Check out something like DefMath.plus.

Gotcha. So if I implement convertFromDef returning a def type is all on me at the moment. I have to do what DefMath does, basically.

@nik9000 Yes, that's correct. Again we are going to rectify this, but wanted to make sure we had something that was usable now.

javanna · 2020-08-25T13:51:17Z

...runtime-fields/src/main/resources/org/elasticsearch/xpack/runtimefields/double_whitelist.txt

-# This import is required to make painless happy and it isn't 100% clear why
+# This whitelist is required to allow painless to build the factory
 class org.elasticsearch.xpack.runtimefields.DoubleScriptFieldScript$Factory @no_import {
 }


I was hoping that this file would go away completely. can you remind what the remaining lines are for?

I'll try one more time to drop it. I tried and it blew up. But I'll try without it entirely.

I think these are required so painless can use the classes that we defined. But these might should all be "implied" because they are part of the script context.

I checked - it is still required.

@stu-elastic and @jdconrad - I think we still need this file because without it painless can't find the factory class. I think that is because is part of a plugin and not in painless's classloader. Or something like that. Is that how the world is supposed to be? Could these classes be implied by default because the context mentions them directly?

Painless has no way to load these classes w/o explicitly stating them as part of SPI.

👍.

Could you dig them out of the ScriptContext, I wonder?

That's something worth looking into. Agreed.

nik9000 · 2020-08-25T16:07:15Z

...time-fields/src/main/java/org/elasticsearch/xpack/runtimefields/DoubleScriptFieldScript.java

-     */
-    public final double[] values() {
-        return values;
+    public static double[] convertFromDouble(double v) {


@stu-elastic and @jdconrad do these look right? I borrowed them from FactoryTests.

I see right now you force the converters to be static - would it be possible to make them non-static on the script?

Not easily because the only "this" pointer we have is through class bindings.

jdconrad

LGTM. Please address the one important note about convertFromDef.

jdconrad · 2020-08-25T16:11:02Z

...time-fields/src/main/java/org/elasticsearch/xpack/runtimefields/DoubleScriptFieldScript.java

+        if (o instanceof Double) {
+            return convertFromDouble(((Double) o).doubleValue());
+        } else {
+            return (double[]) o;


Indy for this is really challenging. We are going to look into it, but allowing you to do def conversions this way ensures we have something for runtime fields now that doesn't use reflection invocation. I would recommend that this cover all numeric cases including byte through long as well. Check out something like DefMath.plus.

jdconrad · 2020-08-25T16:12:36Z

...runtime-fields/src/main/resources/org/elasticsearch/xpack/runtimefields/double_whitelist.txt

-# This import is required to make painless happy and it isn't 100% clear why
+# This whitelist is required to allow painless to build the factory
 class org.elasticsearch.xpack.runtimefields.DoubleScriptFieldScript$Factory @no_import {
 }


Painless has no way to load these classes w/o explicitly stating them as part of SPI.

nik9000 · 2020-08-25T18:50:13Z

Thanks all!

This reverts commit 6d8170c. We've decided to stick with the `value` style functions for runtime fields.

nik9000 added the :Search/Search Search-related issues that do not fall into other categories label Aug 24, 2020

nik9000 requested a review from javanna August 24, 2020 20:14

elasticmachine added the Team:Search Meta label for search team label Aug 24, 2020

nik9000 commented Aug 24, 2020

View reviewed changes

javanna approved these changes Aug 25, 2020

View reviewed changes

nik9000 added 2 commits August 25, 2020 10:45

Unit tests for conversions

a34e57b

Format

0e885f0

nik9000 requested review from jdconrad and stu-elastic August 25, 2020 15:58

javanna mentioned this pull request Aug 25, 2020

Add support for runtime fields #59332

Closed

30 tasks

nik9000 commented Aug 25, 2020

View reviewed changes

jdconrad approved these changes Aug 25, 2020

View reviewed changes

Handle all numbers

3bb6710

nik9000 merged commit 6d8170c into elastic:feature/runtime_fields Aug 25, 2020

nik9000 added a commit that referenced this pull request Aug 31, 2020

Revert "Convert double script to return array (#61504)"

95cdff7

This reverts commit 6d8170c. We've decided to stick with the `value` style functions for runtime fields.

Conversation

nik9000 commented Aug 24, 2020 • edited by javanna Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Aug 24, 2020

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!

javanna 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!

jdconrad Aug 25, 2020 • 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!

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!

jdconrad 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!

nik9000 commented Aug 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nik9000 commented Aug 24, 2020 •

edited by javanna

Loading

jdconrad Aug 25, 2020 •

edited

Loading