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.

Sign up for GitHub

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

Switch default time format for ingest from Joda to Java for v7 #37934

Merged
merged 3 commits into from
Jan 30, 2019
Merged

Switch default time format for ingest from Joda to Java for v7 #37934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a7191e
Switch default time format for ingest from Joda to Java for v7
droberts195 Jan 28, 2019
961f5ef
Merge branch 'master' into java_time_default_for_ingest_in_7
droberts195 Jan 29, 2019
1b636e3
Fix YAML test
droberts195 Jan 29, 2019
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
37 changes: 14 additions & 23 deletions modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/DateFormat.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

package org.elasticsearch.ingest.common;

import org.elasticsearch.common.joda.Joda;
import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.common.time.DateFormatters;
import org.elasticsearch.common.time.DateUtils;
Expand Down Expand Up @@ -73,30 +72,22 @@ private long parseMillis(String date) {
Java {
@Override
Function<String, DateTime> getFunction(String format, DateTimeZone timezone, Locale locale) {
// in case you are wondering why we do not call 'DateFormatter.forPattern(format)' for all cases here, but only for the
// non java time case:
// When the joda date formatter parses a date then a year is always set, so that no fallback can be used, like
// done in the JodaDateFormatter.withYear() code below
// This means that we leave the existing parsing logic in place, but will fall back to the new java date parsing logic, if an
// "8" is prepended to the date format string
int year = LocalDate.now(ZoneOffset.UTC).getYear();

// support the 6.x BWC compatible way of parsing java 8 dates
if (format.startsWith("8")) {
DateFormatter formatter = DateFormatter.forPattern(format)
.withLocale(locale)
.withZone(DateUtils.dateTimeZoneToZoneId(timezone));
return text -> {
ZonedDateTime defaultZonedDateTime = Instant.EPOCH.atZone(ZoneOffset.UTC).withYear(year);
TemporalAccessor accessor = formatter.parse(text);
long millis = DateFormatters.toZonedDateTime(accessor, defaultZonedDateTime).toInstant().toEpochMilli();
return new DateTime(millis, timezone);
};
} else {
DateFormatter formatter = Joda.forPattern(format)
.withYear(year)
.withZone(DateUtils.dateTimeZoneToZoneId(timezone))
.withLocale(locale);
return text -> new DateTime(formatter.parseMillis(text), timezone);
format = format.substring(1);
}

int year = LocalDate.now(ZoneOffset.UTC).getYear();
DateFormatter formatter = DateFormatter.forPattern(format)
.withLocale(locale)
.withZone(DateUtils.dateTimeZoneToZoneId(timezone));
return text -> {
ZonedDateTime defaultZonedDateTime = Instant.EPOCH.atZone(ZoneOffset.UTC).withYear(year);
TemporalAccessor accessor = formatter.parse(text);
long millis = DateFormatters.toZonedDateTime(accessor, defaultZonedDateTime).toInstant().toEpochMilli();
return new DateTime(millis, timezone);
Copy link
Contributor

Choose a reason for hiding this comment

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

we are still returned a joda time class here, that we need to get rid of. I am happy to fix this in a follow up PR though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @spinscale. I'd prefer to leave that for a followup PR as I'm sure removing Joda classes will spread out far and wide through the codebase.

};
}
};

Expand Down
2 changes: 1 addition & 1 deletion ...s/ingest-common/src/main/java/org/elasticsearch/ingest/common/DateIndexNameProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ public DateIndexNameProcessor create(Map<String, Processor.Factory> registry, St
}
List<String> dateFormatStrings = ConfigurationUtils.readOptionalList(TYPE, tag, config, "date_formats");
if (dateFormatStrings == null) {
dateFormatStrings = Collections.singletonList("yyyy-MM-dd'T'HH:mm:ss.SSSZ");
dateFormatStrings = Collections.singletonList("yyyy-MM-dd'T'HH:mm:ss.SSSXX");
}
List<Function<String, DateTime>> dateFormats = new ArrayList<>(dateFormatStrings.size());
for (String format : dateFormatStrings) {
Expand Down
6 changes: 3 additions & 3 deletions ...est-common/src/test/java/org/elasticsearch/ingest/common/DateIndexNameProcessorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@

public class DateIndexNameProcessorTests extends ESTestCase {

public void testJodaPattern() throws Exception {
Function<String, DateTime> function = DateFormat.Java.getFunction("yyyy-MM-dd'T'HH:mm:ss.SSSZ", DateTimeZone.UTC, Locale.ROOT);
public void testJavaPattern() throws Exception {
Function<String, DateTime> function = DateFormat.Java.getFunction("yyyy-MM-dd'T'HH:mm:ss.SSSXX", DateTimeZone.UTC, Locale.ROOT);
DateIndexNameProcessor processor = createProcessor("_field", Collections.singletonList(function),
DateTimeZone.UTC, "events-", "y", "yyyyMMdd");
IngestDocument document = new IngestDocument("_index", "_type", "_id", null, null, null,
Expand Down Expand Up @@ -82,7 +82,7 @@ public void testUnix()throws Exception {
public void testTemplatedFields() throws Exception {
String indexNamePrefix = randomAlphaOfLength(10);
String dateRounding = randomFrom("y", "M", "w", "d", "h", "m", "s");
String indexNameFormat = randomFrom("yyyy-MM-dd'T'HH:mm:ss.SSSZ", "yyyyMMdd", "MM/dd/yyyy");
String indexNameFormat = randomFrom("yyyy-MM-dd'T'HH:mm:ss.SSSXX", "yyyyMMdd", "MM/dd/yyyy");
String date = Integer.toString(randomInt());
Function<String, DateTime> dateTimeFunction = DateFormat.Unix.getFunction(null, DateTimeZone.UTC, null);

Expand Down
21 changes: 10 additions & 11 deletions modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/DateProcessorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,18 +49,19 @@ private TemplateScript.Factory templatize(ZoneId timezone) {
String id = timezone.equals(ZoneOffset.UTC) ? "UTC" : timezone.getId();
return new TestTemplateService.MockTemplateScript.Factory(id);
}
public void testJodaPattern() {

public void testJavaPattern() {
DateProcessor dateProcessor = new DateProcessor(randomAlphaOfLength(10),
templatize(ZoneId.of("Europe/Amsterdam")), templatize(Locale.ENGLISH),
"date_as_string", Collections.singletonList("yyyy dd MM hh:mm:ss"), "date_as_date");
"date_as_string", Collections.singletonList("yyyy dd MM HH:mm:ss"), "date_as_date");
Map<String, Object> document = new HashMap<>();
document.put("date_as_string", "2010 12 06 11:05:15");
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), document);
dateProcessor.execute(ingestDocument);
assertThat(ingestDocument.getFieldValue("date_as_date", String.class), equalTo("2010-06-12T11:05:15.000+02:00"));
}

public void testJodaPatternMultipleFormats() {
public void testJavaPatternMultipleFormats() {
List<String> matchFormats = new ArrayList<>();
matchFormats.add("yyyy dd MM");
matchFormats.add("dd/MM/yyyy");
Expand Down Expand Up @@ -98,7 +99,7 @@ public void testJodaPatternMultipleFormats() {
}
}

public void testInvalidJodaPattern() {
public void testInvalidJavaPattern() {
try {
DateProcessor processor = new DateProcessor(randomAlphaOfLength(10),
templatize(ZoneOffset.UTC), templatize(randomLocale(random())),
Expand All @@ -109,24 +110,22 @@ public void testInvalidJodaPattern() {
fail("date processor execution should have failed");
} catch(IllegalArgumentException e) {
assertThat(e.getMessage(), equalTo("unable to parse date [2010]"));
assertThat(e.getCause().getMessage(), equalTo("Invalid format: [invalid pattern]: Illegal pattern component: i"));
assertThat(e.getCause().getMessage(), equalTo("Invalid format: [invalid pattern]: Unknown pattern letter: i"));
}
}

public void testJodaPatternLocale() {
//TODO investigate if this is a bug in Joda
assumeFalse("Can't run in a FIPS JVM, Joda parse date error", inFipsJvm());
DateProcessor dateProcessor = new DateProcessor(randomAlphaOfLength(10),
public void testJavaPatternLocale() {
DateProcessor dateProcessor = new DateProcessor(randomAlphaOfLength(10),
templatize(ZoneId.of("Europe/Amsterdam")), templatize(Locale.ITALIAN),
"date_as_string", Collections.singletonList("yyyy dd MMM"), "date_as_date");
"date_as_string", Collections.singletonList("yyyy dd MMMM"), "date_as_date");
Map<String, Object> document = new HashMap<>();
document.put("date_as_string", "2010 12 giugno");
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), document);
dateProcessor.execute(ingestDocument);
assertThat(ingestDocument.getFieldValue("date_as_date", String.class), equalTo("2010-06-12T00:00:00.000+02:00"));
}

public void testJodaPatternDefaultYear() {
public void testJavaPatternDefaultYear() {
String format = randomFrom("dd/MM", "8dd/MM");
DateProcessor dateProcessor = new DateProcessor(randomAlphaOfLength(10),
templatize(ZoneId.of("Europe/Amsterdam")), templatize(Locale.ENGLISH),
Expand Down
8 changes: 3 additions & 5 deletions ...h-all-dependencies/src/test/resources/rest-api-spec/test/ingest/20_combine_processors.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
---
"Test logging":
"Test with date processor":
- skip:
version: " - 6.9.99"
reason: pre-7.0.0 will send no warnings
reason: pre-7.0.0 requires the 8 prefix for Java time formats, so would treat the format in this test as a Joda time format
features: "warnings"

- do:
Expand Down Expand Up @@ -33,7 +33,7 @@
"date" : {
"field" : "timestamp",
"target_field" : "timestamp",
"formats" : ["dd/MMM/YYYY:HH:mm:ss Z"]
"formats" : ["dd/MMM/yyyy:HH:mm:ss xx"]
}
},
{
Expand All @@ -46,8 +46,6 @@
- match: { acknowledged: true }

- do:
warnings:
- "Use of 'Y' (year-of-era) will change to 'y' in the next major version of Elasticsearch. Prefix your date format with '8' to use the new specifier."
index:
index: test
type: test
Expand Down