-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Changes from all commits
925d6bb
f42474c
d3d2908
9188b15
1a23214
8f63f76
99b288c
8ad8573
931d2b1
2bd06d9
fc019aa
0650ac8
c37f33d
c599e46
4383c43
cd8a7a7
dee8abe
7a67906
c4b19cf
f3add18
286eb8d
ac773d8
32e0e80
e4d5bcb
c750fd6
7592108
6b95165
4042747
74c9b89
96e843f
055d2c0
b28ce90
efa653b
63e941b
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 |
---|---|---|
|
@@ -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; | ||
|
@@ -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 | ||
|
@@ -52,6 +56,7 @@ | |
* values form multiple documents. | ||
*/ | ||
public abstract class ScriptDocValues<T> extends AbstractList<T> { | ||
|
||
/** | ||
* Set the current doc ID. | ||
*/ | ||
|
@@ -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> { | ||
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. wouldn't is be simpler to have 2 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 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!"); | ||
|
@@ -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 [" | ||
|
@@ -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> { | ||
|
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.
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.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.
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.