Skip to content

Commit

Permalink
Revert "Convert double script to return array (#61504)"
Browse files Browse the repository at this point in the history
This reverts commit 6d8170c. We've
decided to stick with the `value` style functions for runtime fields.
  • Loading branch information
nik9000 committed Aug 31, 2020
1 parent c9bd8c8 commit 95cdff7
Show file tree
Hide file tree
Showing 21 changed files with 103 additions and 175 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,6 @@ private static String painlessToLoadFromSource(String name, String type) {
return null;
}
StringBuilder b = new StringBuilder();
if ("double".equals(type)) {
b.append("List result = new ArrayList();");
}
b.append("def v = source['").append(name).append("'];\n");
b.append("if (v instanceof Iterable) {\n");
b.append(" for (def vv : ((Iterable) v)) {\n");
Expand All @@ -183,9 +180,6 @@ private static String painlessToLoadFromSource(String name, String type) {
b.append(" ").append(emit).append("\n");
b.append(" }\n");
b.append("}\n");
if ("double".equals(type)) {
b.append("return result;");
}
return b.toString();
}

Expand All @@ -194,7 +188,7 @@ private static String painlessToLoadFromSource(String name, String type) {
Map.entry(DateFieldMapper.CONTENT_TYPE, "millis(parse(value.toString()));"),
Map.entry(
NumberType.DOUBLE.typeName(),
"result.add(value instanceof Number ? ((Number) value).doubleValue() : Double.parseDouble(value.toString()));"
"value(value instanceof Number ? ((Number) value).doubleValue() : Double.parseDouble(value.toString()));"
),
Map.entry(KeywordFieldMapper.CONTENT_TYPE, "value(value.toString());"),
Map.entry(IpFieldMapper.CONTENT_TYPE, "stringValue(value.toString());"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ public AbstractLongScriptFieldScript(Map<String, Object> params, SearchLookup se
super(params, searchLookup, ctx);
}

public abstract void execute();

/**
* Execute the script for the provided {@code docId}.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,6 @@ public final Map<String, Object> getSource() {
public final Map<String, ScriptDocValues<?>> getDoc() {
return leafSearchLookup.doc();
}

public abstract void execute();
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ public BooleanScriptFieldScript(Map<String, Object> params, SearchLookup searchL
super(params, searchLookup, ctx);
}

public abstract void execute();

/**
* Execute the script for the provided {@code docId}.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -35,40 +35,55 @@ 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 double[] runForDoc(int docId) {
public final void runForDoc(int docId) {
count = 0;
setDocument(docId);
return execute();
execute();
}

public static double[] convertFromDouble(double v) {
return new double[] { v };
/**
* 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[] convertFromCollection(Collection<?> v) {
double[] result = new double[v.size()];
int i = 0;
for (Object o : v) {
result[i++] = ((Number) o).doubleValue();
/**
* 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);
}
return result;
values[count++] = 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;
public static class Value {
private final DoubleScriptFieldScript script;

public Value(DoubleScriptFieldScript script) {
this.script = script;
}

public void value(double v) {
script.collectValue(v);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,6 @@ public IpScriptFieldScript(Map<String, Object> params, SearchLookup searchLookup
super(params, searchLookup, ctx);
}

public abstract void execute();

/**
* Execute the script for the provided {@code docId}.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ public StringScriptFieldScript(Map<String, Object> params, SearchLookup searchLo
super(params, searchLookup, ctx);
}

public abstract void execute();

/**
* Execute the script for the provided {@code docId}.
* <p>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

public final class ScriptDoubleDocValues extends SortedNumericDoubleValues {
private final DoubleScriptFieldScript script;
private double[] values;
private int cursor;

ScriptDoubleDocValues(DoubleScriptFieldScript script) {
Expand All @@ -23,22 +22,22 @@ public final class ScriptDoubleDocValues extends SortedNumericDoubleValues {

@Override
public boolean advanceExact(int docId) {
values = script.runForDoc(docId);
if (values.length == 0) {
script.runForDoc(docId);
if (script.count() == 0) {
return false;
}
Arrays.sort(values);
Arrays.sort(script.values(), 0, script.count());
cursor = 0;
return true;
}

@Override
public double nextValue() throws IOException {
return values[cursor++];
return script.values()[cursor++];
}

@Override
public int docValueCount() {
return values.length;
return script.count();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ abstract class AbstractDoubleScriptFieldQuery extends AbstractScriptFieldQuery {
/**
* Does the value match this query?
*/
protected abstract boolean matches(double[] values);
protected abstract boolean matches(double[] values, int count);

@Override
public final Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) throws IOException {
Expand All @@ -53,7 +53,8 @@ public Scorer scorer(LeafReaderContext ctx) throws IOException {
TwoPhaseIterator twoPhase = new TwoPhaseIterator(approximation) {
@Override
public boolean matches() throws IOException {
return AbstractDoubleScriptFieldQuery.this.matches(script.runForDoc(approximation().docID()));
script.runForDoc(approximation().docID());
return AbstractDoubleScriptFieldQuery.this.matches(script.values(), script.count());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ public DoubleScriptFieldExistsQuery(Script script, DoubleScriptFieldScript.LeafF
}

@Override
protected boolean matches(double[] values) {
return values.length > 0;
protected boolean matches(double[] values, int count) {
return count > 0;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ public DoubleScriptFieldRangeQuery(
}

@Override
protected boolean matches(double[] values) {
for (double value : values) {
if (lowerValue <= value && value <= upperValue) {
protected boolean matches(double[] values, int count) {
for (int i = 0; i < count; i++) {
if (lowerValue <= values[i] && values[i] <= upperValue) {
return true;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ public DoubleScriptFieldTermQuery(Script script, DoubleScriptFieldScript.LeafFac
}

@Override
protected boolean matches(double[] values) {
for (double value : values) {
if (term == value) {
protected boolean matches(double[] values, int count) {
for (int i = 0; i < count; i++) {
if (term == values[i]) {
return true;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ public DoubleScriptFieldTermsQuery(Script script, DoubleScriptFieldScript.LeafFa
}

@Override
protected boolean matches(double[] values) {
for (double value : values) {
if (terms.contains(Double.doubleToLongBits(value))) {
protected boolean matches(double[] values, int count) {
for (int i = 0; i < count; i++) {
if (terms.contains(Double.doubleToLongBits(values[i]))) {
return true;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
class org.elasticsearch.xpack.runtimefields.DoubleScriptFieldScript @no_import {
}

# This whitelist is required to allow painless to build the factory
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
class org.elasticsearch.xpack.runtimefields.DoubleScriptFieldScript$Factory @no_import {
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,15 @@

import org.elasticsearch.script.ScriptContext;

import java.util.List;

import static org.hamcrest.Matchers.equalTo;

public class DoubleScriptFieldScriptTests extends ScriptFieldScriptTestCase<DoubleScriptFieldScript.Factory> {
public static final DoubleScriptFieldScript.Factory DUMMY = (params, lookup) -> ctx -> new DoubleScriptFieldScript(
params,
lookup,
ctx
) {
@Override
public double[] execute() {
return new double[] { 1.0 };
public void execute() {
new DoubleScriptFieldScript.Value(this).value(1.0);
}
};

Expand All @@ -33,29 +29,4 @@ protected ScriptContext<DoubleScriptFieldScript.Factory> context() {
protected DoubleScriptFieldScript.Factory dummyScript() {
return DUMMY;
}

public void testConvertDouble() {
double v = randomDouble();
assertThat(DoubleScriptFieldScript.convertFromDouble(v), equalTo(new double[] { v }));
assertThat(DoubleScriptFieldScript.convertFromDef(v), equalTo(new double[] { v }));
}

public void testConvertFromCollection() {
double d = randomDouble();
long l = randomLong();
int i = randomInt();
assertThat(DoubleScriptFieldScript.convertFromCollection(List.of(d, l, i)), equalTo(new double[] { d, l, i }));
assertThat(DoubleScriptFieldScript.convertFromDef(List.of(d, l, i)), equalTo(new double[] { d, l, i }));
}

public void testConvertDoubleArrayFromDef() {
double[] a = new double[] { 1, 2, 3 };
assertThat(DoubleScriptFieldScript.convertFromDef(a), equalTo(a));
}

public void testConvertNumberFromDef() {
for (Number n : new Number[] { randomByte(), randomShort(), randomInt(), randomLong(), randomFloat() }) {
assertThat(DoubleScriptFieldScript.convertFromDef(n), equalTo(new double[] { n.doubleValue() }));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -270,27 +270,21 @@ private DoubleScriptFieldScript.Factory factory(String code) {
case "read_foo":
return (params, lookup) -> (ctx) -> new DoubleScriptFieldScript(params, lookup, ctx) {
@Override
public double[] execute() {
List<?> foos = (List<?>) getSource().get("foo");
double[] results = new double[foos.size()];
int i = 0;
for (Object foo : foos) {
results[i++] = ((Number) foo).doubleValue();
public void execute() {
for (Object foo : (List<?>) getSource().get("foo")) {
new DoubleScriptFieldScript.Value(this).value(((Number) foo).doubleValue());
}
return results;
}
};
case "add_param":
return (params, lookup) -> (ctx) -> new DoubleScriptFieldScript(params, lookup, ctx) {
@Override
public double[] execute() {
List<?> foos = (List<?>) getSource().get("foo");
double[] results = new double[foos.size()];
int i = 0;
for (Object foo : foos) {
results[i++] = ((Number) getParams().get("param")).doubleValue() + ((Number) foo).doubleValue();
public void execute() {
for (Object foo : (List<?>) getSource().get("foo")) {
new DoubleScriptFieldScript.Value(this).value(
((Number) foo).doubleValue() + ((Number) getParams().get("param")).doubleValue()
);
}
return results;
}
};
default:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,9 @@ protected DoubleScriptFieldExistsQuery mutate(DoubleScriptFieldExistsQuery orig)

@Override
public void testMatches() {
assertTrue(createTestInstance().matches(new double[] { 1 }));
assertFalse(createTestInstance().matches(new double[0]));
double[] big = new double[between(1, 10000)];
for (int i = 0; i < big.length; i++) {
big[i] = 1.0;
}
assertTrue(createTestInstance().matches(big));
assertTrue(createTestInstance().matches(new double[0], randomIntBetween(1, Integer.MAX_VALUE)));
assertFalse(createTestInstance().matches(new double[0], 0));
assertFalse(createTestInstance().matches(new double[] { 1, 2, 3 }, 0));
}

@Override
Expand Down
Loading

0 comments on commit 95cdff7

Please sign in to comment.