Skip to content
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

Scripting: Conditionally use java time api in scripting #31441

Merged
merged 34 commits into from
Aug 1, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
925d6bb
Scripting: Conditionally use java time api in scripting
rjernst Jun 19, 2018
f42474c
remove unused import
rjernst Jun 19, 2018
d3d2908
fix compile
rjernst Jun 19, 2018
9188b15
Merge branch 'master' into timeapi2
rjernst Jun 20, 2018
1a23214
change setting name and move check into ScriptModule
rjernst Jun 20, 2018
8f63f76
Merge branch 'master' into timeapi2
rjernst Jun 22, 2018
99b288c
Merge branch 'master' into timeapi2
rjernst Jun 27, 2018
8ad8573
fix checkstyle
rjernst Jun 27, 2018
931d2b1
Merge branch 'master' into timeapi2
rjernst Jul 2, 2018
2bd06d9
better tests
rjernst Jul 3, 2018
fc019aa
fix script conversions from date to number
rjernst Jul 3, 2018
0650ac8
more conversions
rjernst Jul 4, 2018
c37f33d
Merge branch 'master' into timeapi2
rjernst Jul 16, 2018
c599e46
fix test class used
rjernst Jul 17, 2018
4383c43
fix test
rjernst Jul 17, 2018
cd8a7a7
Merge branch 'master' into timeapi2
rjernst Jul 19, 2018
dee8abe
fix missing imports
rjernst Jul 19, 2018
7a67906
fix test
rjernst Jul 19, 2018
c4b19cf
Merge branch 'master' into timeapi2
rjernst Jul 24, 2018
f3add18
rework to emit deprecation on each use of script with dates
rjernst Jul 25, 2018
286eb8d
fix checkstyle
rjernst Jul 25, 2018
ac773d8
Merge branch 'master' into timeapi2
rjernst Jul 26, 2018
32e0e80
add doc note and make doc tests use joda time api
rjernst Jul 26, 2018
e4d5bcb
Merge branch 'master' into timeapi2
rjernst Jul 30, 2018
c750fd6
add warning to tests
rjernst Jul 30, 2018
7592108
remove accidentally added back 6.x bwc test
rjernst Jul 30, 2018
6b95165
fix painless tests
rjernst Jul 30, 2018
4042747
fix test
rjernst Jul 31, 2018
74c9b89
fix more tests to use normalized zoneid
rjernst Jul 31, 2018
96e843f
use ZoneOffset.UTC
rjernst Jul 31, 2018
055d2c0
remove unused import
rjernst Jul 31, 2018
b28ce90
another unused import...
rjernst Jul 31, 2018
efa653b
Merge branch 'master' into timeapi2
rjernst Jul 31, 2018
63e941b
and one more
rjernst Jul 31, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -777,11 +777,16 @@ class BuildPlugin implements Plugin<Project> {
systemProperty property.getKey(), property.getValue()
}
}

// TODO: remove this once joda time is removed from scriptin in 7.0
systemProperty 'es.scripting.use_java_time', 'true'

// Set the system keystore/truststore password if we're running tests in a FIPS-140 JVM
if (project.inFipsJvm) {
systemProperty 'javax.net.ssl.trustStorePassword', 'password'
systemProperty 'javax.net.ssl.keyStorePassword', 'password'
}

boolean assertionsEnabled = Boolean.parseBoolean(System.getProperty('tests.asserts', 'true'))
enableSystemAssertions assertionsEnabled
enableAssertions assertionsEnabled
Expand Down
3 changes: 3 additions & 0 deletions docs/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ integTestCluster {
extraConfigFile 'hunspell/en_US/en_US.dic', '../server/src/test/resources/indices/analyze/conf_dir/hunspell/en_US/en_US.dic'
// Whitelist reindexing from the local node so we can test it.
setting 'reindex.remote.whitelist', '127.0.0.1:*'

// TODO: remove this for 7.0, this exists to allow the doc examples in 6.x to continue using the defaults
systemProperty 'es.scripting.use_java_time', 'false'
}

// remove when https://github.com/elastic/elasticsearch/issues/31305 is fixed
Expand Down
7 changes: 6 additions & 1 deletion docs/painless/painless-getting-started.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ POST hockey/player/1/_update
==== Dates

Date fields are exposed as
`ReadableDateTime`
`ReadableDateTime` or
so they support methods like
`getYear`,
and `getDayOfWeek`.
Expand All @@ -220,6 +220,11 @@ GET hockey/_search
}
----------------------------------------------------------------
// CONSOLE
// TEST[warning:The joda time api for doc values is deprecated. Use -Des.scripting.use_java_time=true to use the java time api for date field doc values]

NOTE: Date fields are changing in 7.0 to be exposed as `ZonedDateTime`
from Java 8's time API. To switch to this functionality early,
add `-Des.scripting.use_java_time=true` to `jvm.options`.

[float]
[[modules-scripting-painless-regex]]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@ POST /sales/_search?size=0
--------------------------------------------------
// CONSOLE
// TEST[setup:sales]
// TEST[warning:The joda time api for doc values is deprecated. Use -Des.scripting.use_java_time=true to use the java time api for date field doc values]

Response:

Expand Down
3 changes: 1 addition & 2 deletions modules/lang-painless/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,14 @@
* under the License.
*/



esplugin {
description 'An easy, safe and fast scripting language for Elasticsearch'
classname 'org.elasticsearch.painless.PainlessPlugin'
}

integTestCluster {
module project.project(':modules:mapper-extras')
systemProperty 'es.scripting.use_java_time', 'true'
}

dependencies {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ class org.elasticsearch.index.fielddata.ScriptDocValues$Longs {
}

class org.elasticsearch.index.fielddata.ScriptDocValues$Dates {
org.joda.time.ReadableDateTime get(int)
org.joda.time.ReadableDateTime getValue()
Object get(int)
Object getValue()
List getValues()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ setup:
script_fields:
bar:
script:
source: "doc.date.value.dayOfWeek"
source: "doc.date.value.dayOfWeek.value"

- match: { hits.hits.0.fields.bar.0: 7}

Expand All @@ -123,7 +123,7 @@ setup:
source: >
StringBuilder b = new StringBuilder();
for (def date : doc.dates) {
b.append(" ").append(date.getDayOfWeek());
b.append(" ").append(date.getDayOfWeek().value);
}
return b.toString().trim()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ setup:
field:
script:
source: "doc.date.get(0)"
- match: { hits.hits.0.fields.field.0: '2017-01-01T12:11:12.000Z' }
- match: { hits.hits.0.fields.field.0: '2017-01-01T12:11:12Z' }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we good with this change, as it changes the string representation of dates? I suppose this just calls LocalTime.toString() which only accounts for nanoseconds if they are greater than zero.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is correct. We can mention it in the 7.0 docs when the bwc behavior is removed. I don't think it has any real impact, since with or without the sub second part, the adheres to ISO 8601.


- do:
search:
Expand All @@ -104,7 +104,7 @@ setup:
field:
script:
source: "doc.date.value"
- match: { hits.hits.0.fields.field.0: '2017-01-01T12:11:12.000Z' }
- match: { hits.hits.0.fields.field.0: '2017-01-01T12:11:12Z' }

---
"geo_point":
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

package org.elasticsearch.index.fielddata;


import org.apache.lucene.index.SortedNumericDocValues;
import org.apache.lucene.util.ArrayUtil;
import org.apache.lucene.util.BytesRef;
Expand All @@ -29,18 +28,23 @@
import org.elasticsearch.common.geo.GeoUtils;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.ESLoggerFactory;
import org.joda.time.DateTime;
import org.joda.time.DateTimeZone;
import org.joda.time.MutableDateTime;
import org.joda.time.ReadableDateTime;

import java.io.IOException;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.time.Instant;
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
import java.util.AbstractList;
import java.util.Arrays;
import java.util.Comparator;
import java.util.List;
import java.util.function.Consumer;
import java.util.function.UnaryOperator;

import static org.elasticsearch.common.Booleans.parseBoolean;

/**
* Script level doc values, the assumption is that any implementation will
Expand All @@ -52,6 +56,7 @@
* values form multiple documents.
*/
public abstract class ScriptDocValues<T> extends AbstractList<T> {

/**
* Set the current doc ID.
*/
Expand Down Expand Up @@ -142,31 +147,55 @@ public int size() {
}
}

public static final class Dates extends ScriptDocValues<ReadableDateTime> {
protected static final DeprecationLogger deprecationLogger = new DeprecationLogger(ESLoggerFactory.getLogger(Dates.class));
public static final class Dates extends ScriptDocValues<Object> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't is be simpler to have 2 Dates classes and keep the type safety instead? I mean I understand we would duplicate the code I still kinda like that better and we can put the conditional in a single place?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't actually have type safety already, because painless exposes raw ScriptDocValues (because it can't know for a given field name which ScriptDocValues specific class is to be used at compile time). I think it would also detach the relevant code for little gain (having to do this selection of which Dates class to use inside field data code where these are constructed).


private static final ReadableDateTime EPOCH = new DateTime(0, DateTimeZone.UTC);
/** Whether scripts should expose dates as java time objects instead of joda time. */
private static final boolean USE_JAVA_TIME = parseBoolean(System.getProperty("es.scripting.use_java_time"), false);

private static final DeprecationLogger deprecationLogger = new DeprecationLogger(ESLoggerFactory.getLogger(Dates.class));

private final SortedNumericDocValues in;

/**
* Method call to add deprecation message. Normally this is
* {@link #deprecationLogger} but tests override.
*/
private final Consumer<String> deprecationCallback;

/**
* Values wrapped in {@link MutableDateTime}. Null by default an allocated on first usage so we allocate a reasonably size. We keep
* this array so we don't have allocate new {@link MutableDateTime}s on every usage. Instead we reuse them for every document.
* Whether java time or joda time should be used. This is normally {@link #USE_JAVA_TIME} but tests override it.
*/
private MutableDateTime[] dates;
private final boolean useJavaTime;

/**
* Values wrapped in a date time object. The concrete type depends on the system property {@code es.scripting.use_java_time}.
* When that system property is {@code false}, the date time objects are of type {@link MutableDateTime}. When the system
* property is {@code true}, the date time objects are of type {@link java.time.ZonedDateTime}.
*/
private Object[] dates;
private int count;

/**
* Standard constructor.
*/
public Dates(SortedNumericDocValues in) {
this(in, message -> deprecationLogger.deprecatedAndMaybeLog("scripting_joda_time_deprecation", message), USE_JAVA_TIME);
}

/**
* Constructor for testing with a deprecation callback.
*/
Dates(SortedNumericDocValues in, Consumer<String> deprecationCallback, boolean useJavaTime) {
this.in = in;
this.deprecationCallback = deprecationCallback;
this.useJavaTime = useJavaTime;
}

/**
* Fetch the first field value or 0 millis after epoch if there are no
* in.
*/
public ReadableDateTime getValue() {
public Object getValue() {
if (count == 0) {
throw new IllegalStateException("A document doesn't have a value for a field! " +
"Use doc[<field>].size()==0 to check if a document is missing a field!");
Expand All @@ -175,7 +204,7 @@ public ReadableDateTime getValue() {
}

@Override
public ReadableDateTime get(int index) {
public Object get(int index) {
if (index >= count) {
throw new IndexOutOfBoundsException(
"attempted to fetch the [" + index + "] date when there are only ["
Expand Down Expand Up @@ -206,31 +235,42 @@ void refreshArray() throws IOException {
if (count == 0) {
return;
}
if (dates == null) {
// Happens for the document. We delay allocating dates so we can allocate it with a reasonable size.
dates = new MutableDateTime[count];
for (int i = 0; i < dates.length; i++) {
dates[i] = new MutableDateTime(in.nextValue(), DateTimeZone.UTC);
if (useJavaTime) {
if (dates == null || count > dates.length) {
// Happens for the document. We delay allocating dates so we can allocate it with a reasonable size.
dates = new ZonedDateTime[count];
}
return;
}
if (count > dates.length) {
// Happens when we move to a new document and it has more dates than any documents before it.
MutableDateTime[] backup = dates;
dates = new MutableDateTime[count];
System.arraycopy(backup, 0, dates, 0, backup.length);
for (int i = 0; i < backup.length; i++) {
dates[i].setMillis(in.nextValue());
for (int i = 0; i < count; ++i) {
dates[i] = ZonedDateTime.ofInstant(Instant.ofEpochMilli(in.nextValue()), ZoneOffset.UTC);
}
for (int i = backup.length; i < dates.length; i++) {
} else {
deprecated("The joda time api for doc values is deprecated. Use -Des.scripting.use_java_time=true" +
" to use the java time api for date field doc values");
if (dates == null || count > dates.length) {
// Happens for the document. We delay allocating dates so we can allocate it with a reasonable size.
dates = new MutableDateTime[count];
}
for (int i = 0; i < count; i++) {
dates[i] = new MutableDateTime(in.nextValue(), DateTimeZone.UTC);
}
return;
}
for (int i = 0; i < count; i++) {
dates[i] = new MutableDateTime(in.nextValue(), DateTimeZone.UTC);
}
}

/**
* Log a deprecation log, with the server's permissions, not the permissions of the
* script calling this method. We need to do this to prevent errors when rolling
* the log file.
*/
private void deprecated(String message) {
// Intentionally not calling SpecialPermission.check because this is supposed to be called by scripts
AccessController.doPrivileged(new PrivilegedAction<Void>() {
@Override
public Void run() {
deprecationCallback.accept(message);
return null;
}
});
}
}

public static final class Doubles extends ScriptDocValues<Double> {
Expand Down
11 changes: 5 additions & 6 deletions server/src/main/java/org/elasticsearch/script/ScriptModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@

package org.elasticsearch.script;

import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.plugins.ScriptPlugin;
import org.elasticsearch.search.aggregations.pipeline.movfn.MovingFunctionScript;

import java.util.Collections;
import java.util.HashMap;
import java.util.List;
Expand All @@ -27,12 +32,6 @@
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.plugins.ScriptPlugin;
import org.elasticsearch.search.aggregations.pipeline.movfn.MovingFunctionScript;


/**
* Manages building {@link ScriptService}.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import java.io.IOException;
import java.lang.reflect.Array;
import java.time.ZonedDateTime;
import java.util.Collection;

/**
Expand Down Expand Up @@ -54,6 +55,9 @@ public boolean advanceExact(int target) throws IOException {
} else if (value instanceof ReadableInstant) {
resize(1);
values[0] = ((ReadableInstant) value).getMillis();
} else if (value instanceof ZonedDateTime) {
resize(1);
values[0] = ((ZonedDateTime) value).toInstant().toEpochMilli();
} else if (value.getClass().isArray()) {
int length = Array.getLength(value);
if (length == 0) {
Expand Down Expand Up @@ -89,6 +93,8 @@ private static double toDoubleValue(Object o) {
} else if (o instanceof ReadableInstant) {
// Dates are exposed in scripts as ReadableDateTimes but aggregations want them to be numeric
return ((ReadableInstant) o).getMillis();
} else if (o instanceof ZonedDateTime) {
return ((ZonedDateTime) o).toInstant().toEpochMilli();
} else if (o instanceof Boolean) {
// We do expose boolean fields as boolean in scripts, however aggregations still expect
// that scripts return the same internal representation as regular fields, so boolean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

import java.io.IOException;
import java.lang.reflect.Array;
import java.time.ZonedDateTime;
import java.util.Collection;
import java.util.Iterator;

Expand Down Expand Up @@ -91,6 +92,8 @@ private static long toLongValue(Object o) {
} else if (o instanceof ReadableInstant) {
// Dates are exposed in scripts as ReadableDateTimes but aggregations want them to be numeric
return ((ReadableInstant) o).getMillis();
} else if (o instanceof ZonedDateTime) {
return ((ZonedDateTime) o).toInstant().toEpochMilli();
} else if (o instanceof Boolean) {
// We do expose boolean fields as boolean in scripts, however aggregations still expect
// that scripts return the same internal representation as regular fields, so boolean
Expand Down
Loading