-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Convert double script to return array #61504
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,8 @@ public AbstractLongScriptFieldScript(Map<String, Object> params, SearchLookup se | |
super(params, searchLookup, ctx); | ||
} | ||
|
||
public abstract void execute(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because I'm only doing one of them I have to move the old execute method decalarations into the subclasses. |
||
|
||
/** | ||
* Execute the script for the provided {@code docId}. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,14 +7,14 @@ | |
package org.elasticsearch.xpack.runtimefields; | ||
|
||
import org.apache.lucene.index.LeafReaderContext; | ||
import org.apache.lucene.util.ArrayUtil; | ||
import org.elasticsearch.painless.spi.Whitelist; | ||
import org.elasticsearch.painless.spi.WhitelistLoader; | ||
import org.elasticsearch.script.ScriptContext; | ||
import org.elasticsearch.script.ScriptFactory; | ||
import org.elasticsearch.search.lookup.SearchLookup; | ||
|
||
import java.io.IOException; | ||
import java.util.Collection; | ||
import java.util.List; | ||
import java.util.Map; | ||
|
||
|
@@ -35,55 +35,40 @@ public interface LeafFactory { | |
DoubleScriptFieldScript newInstance(LeafReaderContext ctx) throws IOException; | ||
} | ||
|
||
private double[] values = new double[1]; | ||
private int count; | ||
|
||
public DoubleScriptFieldScript(Map<String, Object> params, SearchLookup searchLookup, LeafReaderContext ctx) { | ||
super(params, searchLookup, ctx); | ||
} | ||
|
||
public abstract double[] execute(); | ||
|
||
/** | ||
* Execute the script for the provided {@code docId}. | ||
*/ | ||
public final void runForDoc(int docId) { | ||
count = 0; | ||
public final double[] runForDoc(int docId) { | ||
setDocument(docId); | ||
execute(); | ||
return execute(); | ||
} | ||
|
||
/** | ||
* Values from the last time {@link #runForDoc(int)} was called. This array | ||
* is mutable and will change with the next call of {@link #runForDoc(int)}. | ||
* It is also oversized and will contain garbage at all indices at and | ||
* above {@link #count()}. | ||
*/ | ||
public final double[] values() { | ||
return values; | ||
public static double[] convertFromDouble(double v) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stu-elastic and @jdconrad do these look right? I borrowed them from I see right now you force the converters to be static - would it be possible to make them non-static on the script? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not easily because the only "this" pointer we have is through class bindings. |
||
return new double[] { v }; | ||
} | ||
|
||
/** | ||
* The number of results produced the last time {@link #runForDoc(int)} was called. | ||
*/ | ||
public final int count() { | ||
return count; | ||
} | ||
|
||
private void collectValue(double v) { | ||
if (values.length < count + 1) { | ||
values = ArrayUtil.grow(values, count + 1); | ||
public static double[] convertFromCollection(Collection<?> v) { | ||
double[] result = new double[v.size()]; | ||
int i = 0; | ||
for (Object o : v) { | ||
result[i++] = ((Number) o).doubleValue(); | ||
} | ||
values[count++] = v; | ||
return result; | ||
} | ||
|
||
public static class Value { | ||
private final DoubleScriptFieldScript script; | ||
|
||
public Value(DoubleScriptFieldScript script) { | ||
this.script = script; | ||
} | ||
|
||
public void value(double v) { | ||
script.collectValue(v); | ||
public static double[] convertFromDef(Object o) { | ||
if (o instanceof Number) { | ||
return convertFromDouble(((Number) o).doubleValue()); | ||
} else if (o instanceof Collection) { | ||
return convertFromCollection((Collection<?>) o); | ||
} else { | ||
return (double[]) o; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😢 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what happens if we do without this conversion? Really bad I guess? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @javanna without it you can't really return @stu-elastic or @jdconrad do we have plans There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We needed to get this in your hands ASAP. We'll be improving it: #61389 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha. So if I implement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nik9000 Yes, that's correct. Again we are going to rectify this, but wanted to make sure we had something that was usable now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤘 |
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,7 +36,7 @@ abstract class AbstractDoubleScriptFieldQuery extends AbstractScriptFieldQuery { | |
/** | ||
* Does the value match this query? | ||
*/ | ||
protected abstract boolean matches(double[] values, int count); | ||
protected abstract boolean matches(double[] values); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need the |
||
|
||
@Override | ||
public final Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) throws IOException { | ||
|
@@ -53,8 +53,7 @@ public Scorer scorer(LeafReaderContext ctx) throws IOException { | |
TwoPhaseIterator twoPhase = new TwoPhaseIterator(approximation) { | ||
@Override | ||
public boolean matches() throws IOException { | ||
script.runForDoc(approximation().docID()); | ||
return AbstractDoubleScriptFieldQuery.this.matches(script.values(), script.count()); | ||
return AbstractDoubleScriptFieldQuery.this.matches(script.runForDoc(approximation().docID())); | ||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,10 +9,6 @@ | |
class org.elasticsearch.xpack.runtimefields.DoubleScriptFieldScript @no_import { | ||
} | ||
|
||
static_import { | ||
void value(org.elasticsearch.xpack.runtimefields.DoubleScriptFieldScript, double) bound_to org.elasticsearch.xpack.runtimefields.DoubleScriptFieldScript$Value | ||
} | ||
|
||
# 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 { | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was hoping that this file would go away completely. can you remind what the remaining lines are for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked - it is still required. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Painless has no way to load these classes w/o explicitly stating them as part of SPI. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍. Could you dig them out of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's something worth looking into. Agreed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
if
is temporary and it'll become all the time once it is done.