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: Rework joda time backcompat #33486

Merged
merged 15 commits into from
Sep 17, 2018
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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 @@ -825,9 +825,6 @@ class BuildPlugin implements Plugin<Project> {
}
}

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

// TODO: remove this once ctx isn't added to update script params in 7.0
systemProperty 'es.scripting.update.ctx_in_params', 'false'

Expand Down
1 change: 0 additions & 1 deletion docs/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ integTestCluster {
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'
systemProperty 'es.scripting.update.ctx_in_params', 'false'
//TODO: remove this once the cname is prepended to the address by default in 7.0
systemProperty 'es.http.cname_in_publish_address', 'true'
Expand Down
5 changes: 0 additions & 5 deletions docs/painless/painless-getting-started.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -220,11 +220,6 @@ 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,7 +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]
// TEST[warning:The return type of [getDayOfWeek()] will change to an enum in 7.0. Use getDayOfWeekEnum().getValue().]
Copy link
Member

Choose a reason for hiding this comment

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

I think you should update this to the ES 7 compatible call.


Response:

Expand Down
1 change: 0 additions & 1 deletion modules/lang-painless/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ esplugin {

integTestCluster {
module project.project(':modules:mapper-extras')
systemProperty 'es.scripting.use_java_time', 'true'
systemProperty 'es.scripting.update.ctx_in_params', 'false'
systemProperty 'es.http.cname_in_publish_address', 'true'
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,7 @@ public final class Whitelist {
"java.util.txt",
"java.util.function.txt",
"java.util.regex.txt",
"java.util.stream.txt",
"joda.time.txt"
"java.util.stream.txt"
};

public static final List<Whitelist> BASE_WHITELISTS =
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,93 @@ class org.elasticsearch.index.fielddata.ScriptDocValues$Longs {
List getValues()
}

class org.elasticsearch.script.JodaCompatibleZonedDateTime {
##### ZonedDateTime methods
int getDayOfMonth()
int getDayOfYear()
int getHour()
LocalDate toLocalDate()
LocalDateTime toLocalDateTime()
int getMinute()
Month getMonth()
int getMonthValue()
int getNano()
int getSecond()
int getYear()
ZonedDateTime minus(TemporalAmount)
ZonedDateTime minus(long,TemporalUnit)
ZonedDateTime minusYears(long)
ZonedDateTime minusMonths(long)
ZonedDateTime minusWeeks(long)
ZonedDateTime minusDays(long)
ZonedDateTime minusHours(long)
ZonedDateTime minusMinutes(long)
ZonedDateTime minusSeconds(long)
ZonedDateTime minusNanos(long)
ZonedDateTime plus(TemporalAmount)
ZonedDateTime plus(long,TemporalUnit)
ZonedDateTime plusDays(long)
ZonedDateTime plusHours(long)
ZonedDateTime plusMinutes(long)
ZonedDateTime plusMonths(long)
ZonedDateTime plusNanos(long)
ZonedDateTime plusSeconds(long)
ZonedDateTime plusWeeks(long)
ZonedDateTime plusYears(long)
Instant toInstant()
Copy link
Contributor

Choose a reason for hiding this comment

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

where is this one coming from?

Copy link
Contributor

Choose a reason for hiding this comment

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

ignore this, misread it as being something for BWC, but it's a new method

OffsetDateTime toOffsetDateTime()
ZonedDateTime truncatedTo(TemporalUnit)
ZonedDateTime with(TemporalAdjuster)
ZonedDateTime with(TemporalField,long)
ZonedDateTime withDayOfMonth(int)
ZonedDateTime withDayOfYear(int)
ZonedDateTime withEarlierOffsetAtOverlap()
ZonedDateTime withFixedOffsetZone()
ZonedDateTime withHour(int)
ZonedDateTime withLaterOffsetAtOverlap()
ZonedDateTime withMinute(int)
ZonedDateTime withMonth(int)
ZonedDateTime withNano(int)
ZonedDateTime withSecond(int)
ZonedDateTime withYear(int)
ZonedDateTime withZoneSameLocal(ZoneId)
ZonedDateTime withZoneSameInstant(ZoneId)

#### Joda methods that exist in java time
boolean equals(Object)
int hashCode()
boolean isAfter(ZonedDateTime)
boolean isBefore(ZonedDateTime)
boolean isEqual(ZonedDateTime)
String toString()

#### Joda time methods
long getMillis()
int getCenturyOfEra()
int getEra()
int getHourOfDay()
int getMillisOfDay()
int getMillisOfSecond()
int getMinuteOfDay()
int getMinuteOfHour()
int getMonthOfYear()
int getSecondOfDay()
int getSecondOfMinute()
int getWeekOfWeekyear()
int getWeekyear()
int getYearOfCentury()
int getYearOfEra()
String toString(String)
String toString(String,Locale)

# conflicting methods
DayOfWeek getDayOfWeekEnum()
int getDayOfWeek()
}

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,8 @@

package org.elasticsearch.painless;

import org.joda.time.DateTime;
import org.joda.time.DateTimeZone;

import java.lang.invoke.LambdaConversionException;
import java.time.Instant;

import static java.util.Collections.singletonMap;
import static org.hamcrest.Matchers.containsString;
Expand Down Expand Up @@ -59,15 +57,15 @@ public void testQualifiedStaticMethodReferenceDef() {
public void testQualifiedVirtualMethodReference() {
long instant = randomLong();
assertEquals(instant, exec(
"List l = [params.d]; return l.stream().mapToLong(org.joda.time.ReadableDateTime::getMillis).sum()",
singletonMap("d", new DateTime(instant, DateTimeZone.UTC)), true));
"List l = [params.d]; return l.stream().mapToLong(Instant::toEpochMilli).sum()",
singletonMap("d", Instant.ofEpochMilli(instant)), true));
}

public void testQualifiedVirtualMethodReferenceDef() {
long instant = randomLong();
assertEquals(instant, exec(
"def l = [params.d]; return l.stream().mapToLong(org.joda.time.ReadableDateTime::getMillis).sum()",
singletonMap("d", new DateTime(instant, DateTimeZone.UTC)), true));
"def l = [params.d]; return l.stream().mapToLong(Instant::toEpochMilli).sum()",
singletonMap("d", Instant.ofEpochMilli(instant)), true));
}

public void testCtorMethodReference() {
Expand Down Expand Up @@ -197,10 +195,10 @@ public void testMethodMissing() {

public void testQualifiedMethodMissing() {
Exception e = expectScriptThrows(IllegalArgumentException.class, () -> {
exec("List l = [2, 1]; l.sort(org.joda.time.ReadableDateTime::bogus); return l.get(0);", false);
exec("List l = [2, 1]; l.sort(java.time.Instant::bogus); return l.get(0);", false);
});
assertThat(e.getMessage(),
containsString("function reference [org.joda.time.ReadableDateTime::bogus/2] matching [java.util.Comparator"));
containsString("function reference [java.time.Instant::bogus/2] matching [java.util.Comparator, compare/2"));
}

public void testClassMissing() {
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.value"
source: "doc.date.value.dayOfWeekEnum.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().value);
b.append(" ").append(date.getDayOfWeekEnum().value);
}
return b.toString().trim()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.elasticsearch.common.text.Text;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.util.concurrent.EsRejectedExecutionException;
import org.elasticsearch.script.JodaCompatibleZonedDateTime;
import org.joda.time.DateTimeZone;
import org.joda.time.ReadableInstant;

Expand Down Expand Up @@ -680,6 +681,14 @@ public final <K, V> void writeMap(final Map<K, V> map, final Writer<K> keyWriter
o.writeString(zonedDateTime.getZone().getId());
o.writeLong(zonedDateTime.toInstant().toEpochMilli());
});
writers.put(JodaCompatibleZonedDateTime.class, (o, v) -> {
// write the joda compatibility datetime as joda datetime
o.writeByte((byte) 13);
final JodaCompatibleZonedDateTime zonedDateTime = (JodaCompatibleZonedDateTime) v;
String zoneId = zonedDateTime.getZonedDateTime().getZone().getId();
Copy link
Contributor

Choose a reason for hiding this comment

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

zoneId is unused here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. This was because the zoneId used by joda doesn't appear to always be the same as java time. Joda does not know how to parse "Z". I'll add special casing for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed f105096

o.writeString(DateTimeZone.UTC.getID());
o.writeLong(zonedDateTime.toInstant().toEpochMilli());
});
WRITERS = Collections.unmodifiableMap(writers);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ public Map<Class<?>, XContentBuilder.Writer> getXContentWriters() {
writers.put(Year.class, (b, v) -> b.value(v.toString()));
writers.put(Duration.class, (b, v) -> b.value(v.toString()));
writers.put(Period.class, (b, v) -> b.value(v.toString()));
//writers.put(JodaCompatibleZonedDateTime.class, XContentBuilder::timeValue);
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean to uncomment this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will be removing it. I added it originally because I thought it was needed due to the api leakage I mentioned in my other comment, but I found a way around that and don't want to expose this compatibility class at all.


writers.put(BytesReference.class, (b, v) -> {
if (v == null) {
Expand Down Expand Up @@ -141,6 +142,8 @@ public Map<Class<?>, Function<Object, Object>> getDateTransformers() {
d -> DEFAULT_FORMATTER.format(ZonedDateTime.ofInstant((java.time.Instant) d, ZoneOffset.UTC)));
transformers.put(LocalDate.class, d -> ((LocalDate) d).toString());
transformers.put(LocalTime.class, d -> LOCAL_TIME_FORMATTER.format((LocalTime) d));
/*transformers.put(JodaCompatibleZonedDateTime.class,
d -> DEFAULT_FORMATTER.format(((JodaCompatibleZonedDateTime) d).getZonedDateTime()));*/
return transformers;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,17 @@
public interface AtomicFieldData extends Accountable, Releasable {

/**
* Returns a "scripting" based values.
* Returns field values for use in scripting.
*/
ScriptDocValues<?> getScriptValues();

/**
* Returns field values for use by returned hits.
*/
default ScriptDocValues<?> getLegacyFieldValues() {
return getScriptValues();
}

/**
* Return a String representation of the values.
*/
Expand Down
Loading