From 9aa70c71a8dba01e853f6e1fe1669c3ebff8bf99 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Tue, 8 Jan 2019 13:50:27 +0100 Subject: [PATCH 1/7] Add benchmark --- .../time/DateFormatterBenchmark.java | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 benchmarks/src/main/java/org/elasticsearch/benchmark/time/DateFormatterBenchmark.java diff --git a/benchmarks/src/main/java/org/elasticsearch/benchmark/time/DateFormatterBenchmark.java b/benchmarks/src/main/java/org/elasticsearch/benchmark/time/DateFormatterBenchmark.java new file mode 100644 index 0000000000000..7c5222b18c48a --- /dev/null +++ b/benchmarks/src/main/java/org/elasticsearch/benchmark/time/DateFormatterBenchmark.java @@ -0,0 +1,57 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.benchmark.time; + +import org.elasticsearch.common.joda.Joda; +import org.elasticsearch.common.time.DateFormatter; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; + +import java.util.concurrent.TimeUnit; + +@Fork(3) +@Warmup(iterations = 10) +@Measurement(iterations = 10) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@State(Scope.Benchmark) +@SuppressWarnings("unused") //invoked by benchmarking framework +public class DateFormatterBenchmark { + + private final DateFormatter javaFormatter = DateFormatter.forPattern("year_month_day||ordinal_date||epoch_millis"); + private final DateFormatter jodaFormatter = Joda.forPattern("year_month_day||ordinal_date||epoch_millis"); + + @Benchmark + public void parseJavaDate() { + javaFormatter.parse("1234567890"); + } + + @Benchmark + public void parseJodaDate() { + jodaFormatter.parse("1234567890"); + } +} + From 7c90a1375a21c579d8eb688f3cc985ffd2a53e7d Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Tue, 8 Jan 2019 13:50:35 +0100 Subject: [PATCH 2/7] Use java time API instead of exception handling when several formatters are used, the existing way of parsing those is to throw an exception catch it, and try the next one. This is is considerably slower than the approach taken in joda time, so that indexing is reduced when a date format like `x||y` is used and y is the date format being used. This commit now uses the java API to parse the date by appending the date time formatters to each other and does not rely on exception handling. --- .../common/time/DateFormatter.java | 2 +- .../common/time/DateFormatters.java | 96 ++----------------- .../common/time/JavaDateFormatter.java | 66 +++++-------- .../joda/JavaJodaTimeDuellingTests.java | 2 - .../common/time/DateFormattersTests.java | 38 ++------ 5 files changed, 40 insertions(+), 164 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/time/DateFormatter.java b/server/src/main/java/org/elasticsearch/common/time/DateFormatter.java index 49c5e7626072b..e89317ad288c0 100644 --- a/server/src/main/java/org/elasticsearch/common/time/DateFormatter.java +++ b/server/src/main/java/org/elasticsearch/common/time/DateFormatter.java @@ -145,7 +145,7 @@ static DateFormatter forPattern(String input) { if (formatters.size() == 1) { return formatters.get(0); } - return new DateFormatters.MergedDateFormatter(input, formatters); + return DateFormatters.merge(input, formatters); } } diff --git a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java index a9d953248a2ce..8b6ce150b408b 100644 --- a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java +++ b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java @@ -19,19 +19,16 @@ package org.elasticsearch.common.time; -import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.Strings; import java.time.DateTimeException; import java.time.DayOfWeek; import java.time.Instant; import java.time.LocalDate; -import java.time.ZoneId; import java.time.ZoneOffset; import java.time.ZonedDateTime; import java.time.format.DateTimeFormatter; import java.time.format.DateTimeFormatterBuilder; -import java.time.format.DateTimeParseException; import java.time.format.ResolverStyle; import java.time.format.SignStyle; import java.time.temporal.ChronoField; @@ -39,10 +36,9 @@ import java.time.temporal.TemporalAccessor; import java.time.temporal.TemporalAdjusters; import java.time.temporal.WeekFields; -import java.util.Collections; +import java.util.ArrayList; import java.util.List; import java.util.Locale; -import java.util.stream.Collectors; import static java.time.temporal.ChronoField.DAY_OF_MONTH; import static java.time.temporal.ChronoField.DAY_OF_WEEK; @@ -1447,90 +1443,18 @@ public static DateFormatter forPattern(String input) { } } - static class MergedDateFormatter implements DateFormatter { + static JavaDateFormatter merge(String pattern, List formatters) { + assert formatters.size() > 0; - private final String pattern; - // package private for tests - final List formatters; - private final List dateMathParsers; - - MergedDateFormatter(String pattern, List formatters) { - assert formatters.size() > 0; - this.pattern = pattern; - this.formatters = Collections.unmodifiableList(formatters); - this.dateMathParsers = formatters.stream().map(DateFormatter::toDateMathParser).collect(Collectors.toList()); + List dateTimeFormatters = new ArrayList<>(); + for (DateFormatter formatter : formatters) { + assert formatter instanceof JavaDateFormatter; + JavaDateFormatter javaDateFormatter = (JavaDateFormatter) formatter; + DateTimeFormatter dateTimeFormatter = javaDateFormatter.getParser(); + dateTimeFormatters.add(dateTimeFormatter); } - @Override - public TemporalAccessor parse(String input) { - IllegalArgumentException failure = null; - for (DateFormatter formatter : formatters) { - try { - return formatter.parse(input); - // TODO: remove DateTimeParseException when JavaDateFormatter throws IAE - } catch (IllegalArgumentException | DateTimeParseException e) { - if (failure == null) { - // wrap so the entire multi format is in the message - failure = new IllegalArgumentException("failed to parse date field [" + input + "] with format [" + pattern + "]", - e); - } else { - failure.addSuppressed(e); - } - } - } - throw failure; - } - - @Override - public DateFormatter withZone(ZoneId zoneId) { - return new MergedDateFormatter(pattern, formatters.stream().map(f -> f.withZone(zoneId)).collect(Collectors.toList())); - } - - @Override - public DateFormatter withLocale(Locale locale) { - return new MergedDateFormatter(pattern, formatters.stream().map(f -> f.withLocale(locale)).collect(Collectors.toList())); - } - - @Override - public String format(TemporalAccessor accessor) { - return formatters.get(0).format(accessor); - } - - @Override - public String pattern() { - return pattern; - } - - @Override - public Locale locale() { - return formatters.get(0).locale(); - } - - @Override - public ZoneId zone() { - return formatters.get(0).zone(); - } - - @Override - public DateMathParser toDateMathParser() { - return (text, now, roundUp, tz) -> { - ElasticsearchParseException failure = null; - for (DateMathParser parser : dateMathParsers) { - try { - return parser.parse(text, now, roundUp, tz); - } catch (ElasticsearchParseException e) { - if (failure == null) { - // wrap so the entire multi format is in the message - failure = new ElasticsearchParseException("failed to parse date field [" + text + "] with format [" - + pattern + "]", e); - } else { - failure.addSuppressed(e); - } - } - } - throw failure; - }; - } + return new JavaDateFormatter(pattern, dateTimeFormatters.get(0), dateTimeFormatters.toArray(new DateTimeFormatter[]{})); } private static final ZonedDateTime EPOCH_ZONED_DATE_TIME = Instant.EPOCH.atZone(ZoneOffset.UTC); diff --git a/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java b/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java index 68e2cfd4fe317..7c73294f85cfe 100644 --- a/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java +++ b/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java @@ -19,10 +19,11 @@ package org.elasticsearch.common.time; +import org.elasticsearch.common.Strings; + import java.time.ZoneId; import java.time.format.DateTimeFormatter; import java.time.format.DateTimeFormatterBuilder; -import java.time.format.DateTimeParseException; import java.time.temporal.ChronoField; import java.time.temporal.TemporalAccessor; import java.time.temporal.TemporalField; @@ -47,7 +48,7 @@ class JavaDateFormatter implements DateFormatter { private final String format; private final DateTimeFormatter printer; - private final DateTimeFormatter[] parsers; + private final DateTimeFormatter parser; JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeFormatter... parsers) { if (printer == null) { @@ -62,61 +63,50 @@ class JavaDateFormatter implements DateFormatter { throw new IllegalArgumentException("formatters must have the same locale"); } if (parsers.length == 0) { - this.parsers = new DateTimeFormatter[]{printer}; + this.parser = printer; + } else if (parsers.length == 1) { + this.parser = parsers[0]; } else { - this.parsers = parsers; + DateTimeFormatterBuilder builder = new DateTimeFormatterBuilder(); + for (DateTimeFormatter parser : parsers) { + builder.appendOptional(parser); + } + this.parser = builder.toFormatter(Locale.ROOT); } this.format = format; this.printer = printer; } + public DateTimeFormatter getParser() { + return parser; + } + @Override public TemporalAccessor parse(String input) { - DateTimeParseException failure = null; - for (int i = 0; i < parsers.length; i++) { - try { - return parsers[i].parse(input); - } catch (DateTimeParseException e) { - if (failure == null) { - failure = e; - } else { - failure.addSuppressed(e); - } - } + if (Strings.isNullOrEmpty(input)) { + throw new IllegalArgumentException("cannot parse empty date"); } - - // ensure that all parsers exceptions are returned instead of only the last one - throw failure; + return parser.parse(input); } @Override public DateFormatter withZone(ZoneId zoneId) { // shortcurt to not create new objects unnecessarily - if (zoneId.equals(parsers[0].getZone())) { + if (zoneId.equals(parser.getZone())) { return this; } - final DateTimeFormatter[] parsersWithZone = new DateTimeFormatter[parsers.length]; - for (int i = 0; i < parsers.length; i++) { - parsersWithZone[i] = parsers[i].withZone(zoneId); - } - - return new JavaDateFormatter(format, printer.withZone(zoneId), parsersWithZone); + return new JavaDateFormatter(format, printer.withZone(zoneId), parser.withZone(zoneId)); } @Override public DateFormatter withLocale(Locale locale) { // shortcurt to not create new objects unnecessarily - if (locale.equals(parsers[0].getLocale())) { + if (locale.equals(parser.getLocale())) { return this; } - final DateTimeFormatter[] parsersWithZone = new DateTimeFormatter[parsers.length]; - for (int i = 0; i < parsers.length; i++) { - parsersWithZone[i] = parsers[i].withLocale(locale); - } - - return new JavaDateFormatter(format, printer.withLocale(locale), parsersWithZone); + return new JavaDateFormatter(format, printer.withLocale(locale), parser.withLocale(locale)); } @Override @@ -132,17 +122,7 @@ public String pattern() { JavaDateFormatter parseDefaulting(Map fields) { final DateTimeFormatterBuilder parseDefaultingBuilder = new DateTimeFormatterBuilder().append(printer); fields.forEach(parseDefaultingBuilder::parseDefaulting); - if (parsers.length == 1 && parsers[0].equals(printer)) { - return new JavaDateFormatter(format, parseDefaultingBuilder.toFormatter(Locale.ROOT)); - } else { - final DateTimeFormatter[] parsersWithDefaulting = new DateTimeFormatter[parsers.length]; - for (int i = 0; i < parsers.length; i++) { - DateTimeFormatterBuilder builder = new DateTimeFormatterBuilder().append(parsers[i]); - fields.forEach(builder::parseDefaulting); - parsersWithDefaulting[i] = builder.toFormatter(Locale.ROOT); - } - return new JavaDateFormatter(format, parseDefaultingBuilder.toFormatter(Locale.ROOT), parsersWithDefaulting); - } + return new JavaDateFormatter(format, parseDefaultingBuilder.toFormatter(Locale.ROOT)); } @Override diff --git a/server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java b/server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java index 23674ec85b44f..20ab9689c7f6d 100644 --- a/server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java +++ b/server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java @@ -72,8 +72,6 @@ public void testCustomTimeFormats() { public void testDuellingFormatsValidParsing() { assertSameDate("1522332219", "epoch_second"); - assertSameDate("1522332219.", "epoch_second"); - assertSameDate("1522332219.0", "epoch_second"); assertSameDate("0", "epoch_second"); assertSameDate("1", "epoch_second"); assertSameDate("1522332219321", "epoch_millis"); diff --git a/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java b/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java index b60e6b27eca03..e68acf6da3984 100644 --- a/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java +++ b/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java @@ -42,21 +42,11 @@ public class DateFormattersTests extends ESTestCase { // as this feature is supported it also makes sense to make it exact public void testEpochMillisParser() { DateFormatter formatter = DateFormatters.forPattern("epoch_millis"); - { - Instant instant = Instant.from(formatter.parse("12345.6789")); - assertThat(instant.getEpochSecond(), is(12L)); - assertThat(instant.getNano(), is(345_678_900)); - } { Instant instant = Instant.from(formatter.parse("12345")); assertThat(instant.getEpochSecond(), is(12L)); assertThat(instant.getNano(), is(345_000_000)); } - { - Instant instant = Instant.from(formatter.parse("12345.")); - assertThat(instant.getEpochSecond(), is(12L)); - assertThat(instant.getNano(), is(345_000_000)); - } { Instant instant = Instant.from(formatter.parse("0")); assertThat(instant.getEpochSecond(), is(0L)); @@ -79,25 +69,12 @@ public void testEpochMilliParser() { public void testEpochSecondParser() { DateFormatter formatter = DateFormatters.forPattern("epoch_second"); - assertThat(Instant.from(formatter.parse("1234.567")).toEpochMilli(), is(1234567L)); - assertThat(Instant.from(formatter.parse("1234.")).getNano(), is(0)); - assertThat(Instant.from(formatter.parse("1234.")).getEpochSecond(), is(1234L)); - assertThat(Instant.from(formatter.parse("1234.1")).getNano(), is(100_000_000)); - assertThat(Instant.from(formatter.parse("1234.12")).getNano(), is(120_000_000)); - assertThat(Instant.from(formatter.parse("1234.123")).getNano(), is(123_000_000)); - assertThat(Instant.from(formatter.parse("1234.1234")).getNano(), is(123_400_000)); - assertThat(Instant.from(formatter.parse("1234.12345")).getNano(), is(123_450_000)); - assertThat(Instant.from(formatter.parse("1234.123456")).getNano(), is(123_456_000)); - assertThat(Instant.from(formatter.parse("1234.1234567")).getNano(), is(123_456_700)); - assertThat(Instant.from(formatter.parse("1234.12345678")).getNano(), is(123_456_780)); - assertThat(Instant.from(formatter.parse("1234.123456789")).getNano(), is(123_456_789)); - - DateTimeParseException e = expectThrows(DateTimeParseException.class, () -> formatter.parse("1234.1234567890")); - assertThat(e.getMessage(), is("Text '1234.1234567890' could not be parsed, unparsed text found at index 4")); - e = expectThrows(DateTimeParseException.class, () -> formatter.parse("1234.123456789013221")); - assertThat(e.getMessage(), is("Text '1234.123456789013221' could not be parsed, unparsed text found at index 4")); + DateTimeParseException e = expectThrows(DateTimeParseException.class, () -> formatter.parse("1234.1")); + assertThat(e.getMessage(), is("Text '1234.1' could not be parsed, unparsed text found at index 4")); + e = expectThrows(DateTimeParseException.class, () -> formatter.parse("1234.")); + assertThat(e.getMessage(), is("Text '1234.' could not be parsed, unparsed text found at index 4")); e = expectThrows(DateTimeParseException.class, () -> formatter.parse("abc")); - assertThat(e.getMessage(), is("Text 'abc' could not be parsed at index 0")); + assertThat(e.getMessage(), is("Text 'abc' could not be parsed, unparsed text found at index 0")); e = expectThrows(DateTimeParseException.class, () -> formatter.parse("1234.abc")); assertThat(e.getMessage(), is("Text '1234.abc' could not be parsed, unparsed text found at index 4")); } @@ -157,9 +134,6 @@ public void testForceJava8() { assertThat(DateFormatter.forPattern("8date_optional_time"), instanceOf(JavaDateFormatter.class)); // named formats too DateFormatter formatter = DateFormatter.forPattern("8date_optional_time||ww-MM-dd"); - assertThat(formatter, instanceOf(DateFormatters.MergedDateFormatter.class)); - DateFormatters.MergedDateFormatter mergedFormatter = (DateFormatters.MergedDateFormatter) formatter; - assertThat(mergedFormatter.formatters.get(0), instanceOf(JavaDateFormatter.class)); - assertThat(mergedFormatter.formatters.get(1), instanceOf(JavaDateFormatter.class)); + assertThat(formatter, instanceOf(JavaDateFormatter.class)); } } From 56d1d07df2b5df878d006cf5ac934faf183d0068 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Tue, 8 Jan 2019 13:56:19 +0100 Subject: [PATCH 3/7] fix benchmark --- .../elasticsearch/benchmark/time/DateFormatterBenchmark.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benchmarks/src/main/java/org/elasticsearch/benchmark/time/DateFormatterBenchmark.java b/benchmarks/src/main/java/org/elasticsearch/benchmark/time/DateFormatterBenchmark.java index 7c5222b18c48a..df4167ff503ed 100644 --- a/benchmarks/src/main/java/org/elasticsearch/benchmark/time/DateFormatterBenchmark.java +++ b/benchmarks/src/main/java/org/elasticsearch/benchmark/time/DateFormatterBenchmark.java @@ -41,7 +41,7 @@ @SuppressWarnings("unused") //invoked by benchmarking framework public class DateFormatterBenchmark { - private final DateFormatter javaFormatter = DateFormatter.forPattern("year_month_day||ordinal_date||epoch_millis"); + private final DateFormatter javaFormatter = DateFormatter.forPattern("8year_month_day||ordinal_date||epoch_millis"); private final DateFormatter jodaFormatter = Joda.forPattern("year_month_day||ordinal_date||epoch_millis"); @Benchmark From 51511a366e834f0390d24d97aa10a99b46489477 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Wed, 9 Jan 2019 09:40:04 +0100 Subject: [PATCH 4/7] fix tests by changing formatter, also expose printer --- .../common/time/DateFormatters.java | 22 +++++++++---------- .../common/time/JavaDateFormatter.java | 6 ++++- .../common/time/DateFormattersTests.java | 9 ++++++++ 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java index 8b6ce150b408b..64dc99eefb05f 100644 --- a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java +++ b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java @@ -118,29 +118,25 @@ public class DateFormatters { .optionalStart() .appendZoneOrOffsetId() .optionalEnd() + .optionalStart() + .appendOffset("+HHmm", "Z") + .optionalEnd() .optionalEnd() .toFormatter(Locale.ROOT); - private static final DateTimeFormatter STRICT_DATE_OPTIONAL_TIME_FORMATTER_WITH_NANOS_2 = new DateTimeFormatterBuilder() + private static final DateTimeFormatter STRICT_DATE_OPTIONAL_TIME_PRINTER = new DateTimeFormatterBuilder() .append(STRICT_YEAR_MONTH_DAY_FORMATTER) - .optionalStart() .appendLiteral('T') .append(STRICT_HOUR_MINUTE_SECOND_FORMATTER) - .optionalStart() .appendFraction(NANO_OF_SECOND, 3, 9, true) - .optionalEnd() - .optionalStart() - .appendOffset("+HHmm", "Z") - .optionalEnd() - .optionalEnd() + .appendZoneOrOffsetId() .toFormatter(Locale.ROOT); /** * Returns a generic ISO datetime parser where the date is mandatory and the time is optional with nanosecond resolution. */ private static final DateFormatter STRICT_DATE_OPTIONAL_TIME_NANOS = new JavaDateFormatter("strict_date_optional_time_nanos", - STRICT_DATE_OPTIONAL_TIME_FORMATTER_WITH_NANOS_1, - STRICT_DATE_OPTIONAL_TIME_FORMATTER_WITH_NANOS_1, STRICT_DATE_OPTIONAL_TIME_FORMATTER_WITH_NANOS_2); + STRICT_DATE_OPTIONAL_TIME_PRINTER, STRICT_DATE_OPTIONAL_TIME_FORMATTER_WITH_NANOS_1); ///////////////////////////////////////// // @@ -1447,14 +1443,18 @@ static JavaDateFormatter merge(String pattern, List formatters) { assert formatters.size() > 0; List dateTimeFormatters = new ArrayList<>(); + DateTimeFormatter printer = null; for (DateFormatter formatter : formatters) { assert formatter instanceof JavaDateFormatter; JavaDateFormatter javaDateFormatter = (JavaDateFormatter) formatter; DateTimeFormatter dateTimeFormatter = javaDateFormatter.getParser(); + if (printer == null) { + printer = javaDateFormatter.getPrinter(); + } dateTimeFormatters.add(dateTimeFormatter); } - return new JavaDateFormatter(pattern, dateTimeFormatters.get(0), dateTimeFormatters.toArray(new DateTimeFormatter[]{})); + return new JavaDateFormatter(pattern, printer, dateTimeFormatters.toArray(new DateTimeFormatter[]{})); } private static final ZonedDateTime EPOCH_ZONED_DATE_TIME = Instant.EPOCH.atZone(ZoneOffset.UTC); diff --git a/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java b/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java index 7c73294f85cfe..0fce14b764ef1 100644 --- a/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java +++ b/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java @@ -77,10 +77,14 @@ class JavaDateFormatter implements DateFormatter { this.printer = printer; } - public DateTimeFormatter getParser() { + DateTimeFormatter getParser() { return parser; } + DateTimeFormatter getPrinter() { + return printer; + } + @Override public TemporalAccessor parse(String input) { if (Strings.isNullOrEmpty(input)) { diff --git a/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java b/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java index e68acf6da3984..0478e541cd7a2 100644 --- a/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java +++ b/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java @@ -23,6 +23,7 @@ import java.time.Instant; import java.time.ZoneId; +import java.time.ZonedDateTime; import java.time.format.DateTimeParseException; import java.time.temporal.TemporalAccessor; import java.util.Locale; @@ -86,6 +87,14 @@ public void testEpochMilliParsersWithDifferentFormatters() { assertThat(formatter.pattern(), is("strict_date_optional_time||epoch_millis")); } + public void testParsersWithMultipleInternalFormats() throws Exception { + ZonedDateTime first = DateFormatters.toZonedDateTime( + DateFormatters.forPattern("strict_date_optional_time_nanos").parse("2018-05-15T17:14:56+0100")); + ZonedDateTime second = DateFormatters.toZonedDateTime( + DateFormatters.forPattern("strict_date_optional_time_nanos").parse("2018-05-15T17:14:56+01:00")); + assertThat(first, is(second)); + } + public void testLocales() { assertThat(DateFormatters.forPattern("strict_date_optional_time").locale(), is(Locale.ROOT)); Locale locale = randomLocale(random()); From 86f19bc98f8b37af36f44589378062f90f116aaf Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Wed, 9 Jan 2019 12:49:48 +0100 Subject: [PATCH 5/7] restore optional printing logic to fix tests --- .../main/java/org/elasticsearch/common/time/DateFormatters.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java index 64dc99eefb05f..b0b2a3c8f0366 100644 --- a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java +++ b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java @@ -129,7 +129,9 @@ public class DateFormatters { .appendLiteral('T') .append(STRICT_HOUR_MINUTE_SECOND_FORMATTER) .appendFraction(NANO_OF_SECOND, 3, 9, true) + .optionalStart() .appendZoneOrOffsetId() + .optionalEnd() .toFormatter(Locale.ROOT); /** From 26e8646c9fc2c02c6574accd117dce22d948d1b5 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Thu, 10 Jan 2019 10:43:42 +0100 Subject: [PATCH 6/7] fix tests --- .../common/time/DateFormatters.java | 58 +++++-------------- 1 file changed, 13 insertions(+), 45 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java index 16502a8d5fa7b..ddc16d0656b24 100644 --- a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java +++ b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java @@ -74,21 +74,17 @@ public class DateFormatters { .appendValue(SECOND_OF_MINUTE, 2, 2, SignStyle.NOT_NEGATIVE) .toFormatter(Locale.ROOT); - private static final DateTimeFormatter STRICT_DATE_OPTIONAL_TIME_FORMATTER_1 = new DateTimeFormatterBuilder() + private static final DateTimeFormatter STRICT_DATE_OPTIONAL_TIME_PRINTER = new DateTimeFormatterBuilder() .append(STRICT_YEAR_MONTH_DAY_FORMATTER) - .optionalStart() .appendLiteral('T') .append(STRICT_HOUR_MINUTE_SECOND_FORMATTER) - .optionalStart() - .appendFraction(MILLI_OF_SECOND, 3, 3, true) - .optionalEnd() + .appendFraction(NANO_OF_SECOND, 3, 9, true) .optionalStart() .appendZoneOrOffsetId() .optionalEnd() - .optionalEnd() .toFormatter(Locale.ROOT); - private static final DateTimeFormatter STRICT_DATE_OPTIONAL_TIME_FORMATTER_2 = new DateTimeFormatterBuilder() + private static final DateTimeFormatter STRICT_DATE_OPTIONAL_TIME_FORMATTER = new DateTimeFormatterBuilder() .append(STRICT_YEAR_MONTH_DAY_FORMATTER) .optionalStart() .appendLiteral('T') @@ -97,7 +93,10 @@ public class DateFormatters { .appendFraction(MILLI_OF_SECOND, 3, 3, true) .optionalEnd() .optionalStart() - .appendOffset("+HHmm", "Z") + .appendZoneOrOffsetId() + .optionalEnd() + .optionalStart() + .append(TIME_ZONE_FORMATTER_NO_COLON) .optionalEnd() .optionalEnd() .toFormatter(Locale.ROOT); @@ -106,10 +105,9 @@ public class DateFormatters { * Returns a generic ISO datetime parser where the date is mandatory and the time is optional. */ private static final DateFormatter STRICT_DATE_OPTIONAL_TIME = - new JavaDateFormatter("strict_date_optional_time", STRICT_DATE_OPTIONAL_TIME_FORMATTER_1, - STRICT_DATE_OPTIONAL_TIME_FORMATTER_1, STRICT_DATE_OPTIONAL_TIME_FORMATTER_2); + new JavaDateFormatter("strict_date_optional_time", STRICT_DATE_OPTIONAL_TIME_PRINTER, STRICT_DATE_OPTIONAL_TIME_FORMATTER); - private static final DateTimeFormatter STRICT_DATE_OPTIONAL_TIME_FORMATTER_WITH_NANOS_1 = new DateTimeFormatterBuilder() + private static final DateTimeFormatter STRICT_DATE_OPTIONAL_TIME_FORMATTER_WITH_NANOS = new DateTimeFormatterBuilder() .append(STRICT_YEAR_MONTH_DAY_FORMATTER) .optionalStart() .appendLiteral('T') @@ -121,18 +119,8 @@ public class DateFormatters { .appendZoneOrOffsetId() .optionalEnd() .optionalStart() - .appendOffset("+HHmm", "Z") - .optionalEnd() + .append(TIME_ZONE_FORMATTER_NO_COLON) .optionalEnd() - .toFormatter(Locale.ROOT); - - private static final DateTimeFormatter STRICT_DATE_OPTIONAL_TIME_PRINTER = new DateTimeFormatterBuilder() - .append(STRICT_YEAR_MONTH_DAY_FORMATTER) - .appendLiteral('T') - .append(STRICT_HOUR_MINUTE_SECOND_FORMATTER) - .appendFraction(NANO_OF_SECOND, 3, 9, true) - .optionalStart() - .appendZoneOrOffsetId() .optionalEnd() .toFormatter(Locale.ROOT); @@ -140,7 +128,7 @@ public class DateFormatters { * Returns a generic ISO datetime parser where the date is mandatory and the time is optional with nanosecond resolution. */ private static final DateFormatter STRICT_DATE_OPTIONAL_TIME_NANOS = new JavaDateFormatter("strict_date_optional_time_nanos", - STRICT_DATE_OPTIONAL_TIME_PRINTER, STRICT_DATE_OPTIONAL_TIME_FORMATTER_WITH_NANOS_1); + STRICT_DATE_OPTIONAL_TIME_PRINTER, STRICT_DATE_OPTIONAL_TIME_FORMATTER_WITH_NANOS); ///////////////////////////////////////// // @@ -816,7 +804,7 @@ public class DateFormatters { * yyyy-MM-dd'T'HH:mm:ss.SSSZ */ private static final DateFormatter DATE_OPTIONAL_TIME = new JavaDateFormatter("date_optional_time", - STRICT_DATE_OPTIONAL_TIME_FORMATTER_1, + STRICT_DATE_OPTIONAL_TIME_PRINTER, new DateTimeFormatterBuilder() .append(DATE_FORMATTER) .optionalStart() @@ -834,26 +822,6 @@ public class DateFormatters { .appendFraction(MILLI_OF_SECOND, 1, 3, true) .optionalEnd() .optionalStart().appendZoneOrOffsetId().optionalEnd() - .optionalEnd() - .optionalEnd() - .optionalEnd() - .toFormatter(Locale.ROOT), - new DateTimeFormatterBuilder() - .append(DATE_FORMATTER) - .optionalStart() - .appendLiteral('T') - .optionalStart() - .appendValue(HOUR_OF_DAY, 1, 2, SignStyle.NOT_NEGATIVE) - .optionalStart() - .appendLiteral(':') - .appendValue(MINUTE_OF_HOUR, 1, 2, SignStyle.NOT_NEGATIVE) - .optionalStart() - .appendLiteral(':') - .appendValue(SECOND_OF_MINUTE, 1, 2, SignStyle.NOT_NEGATIVE) - .optionalEnd() - .optionalStart() - .appendFraction(MILLI_OF_SECOND, 1, 3, true) - .optionalEnd() .optionalStart().appendOffset("+HHmm", "Z").optionalEnd() .optionalEnd() .optionalEnd() @@ -1004,7 +972,7 @@ public class DateFormatters { * (yyyy-MM-dd'T'HH:mm:ss.SSSZZ). */ private static final DateFormatter DATE_TIME = new JavaDateFormatter("date_time", - STRICT_DATE_OPTIONAL_TIME_FORMATTER_1, + STRICT_DATE_OPTIONAL_TIME_PRINTER, new DateTimeFormatterBuilder().append(DATE_TIME_FORMATTER).appendZoneOrOffsetId().toFormatter(Locale.ROOT), new DateTimeFormatterBuilder().append(DATE_TIME_FORMATTER).append(TIME_ZONE_FORMATTER_NO_COLON).toFormatter(Locale.ROOT) ); From 6363d769da3025509e10dad7928ac540ce4b3b37 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Thu, 10 Jan 2019 13:49:29 +0100 Subject: [PATCH 7/7] incorporate review comments --- .../benchmark/time/DateFormatterBenchmark.java | 9 +++++---- .../org/elasticsearch/common/time/DateFormatters.java | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/benchmarks/src/main/java/org/elasticsearch/benchmark/time/DateFormatterBenchmark.java b/benchmarks/src/main/java/org/elasticsearch/benchmark/time/DateFormatterBenchmark.java index df4167ff503ed..b30b3ada0ab64 100644 --- a/benchmarks/src/main/java/org/elasticsearch/benchmark/time/DateFormatterBenchmark.java +++ b/benchmarks/src/main/java/org/elasticsearch/benchmark/time/DateFormatterBenchmark.java @@ -30,6 +30,7 @@ import org.openjdk.jmh.annotations.State; import org.openjdk.jmh.annotations.Warmup; +import java.time.temporal.TemporalAccessor; import java.util.concurrent.TimeUnit; @Fork(3) @@ -45,13 +46,13 @@ public class DateFormatterBenchmark { private final DateFormatter jodaFormatter = Joda.forPattern("year_month_day||ordinal_date||epoch_millis"); @Benchmark - public void parseJavaDate() { - javaFormatter.parse("1234567890"); + public TemporalAccessor parseJavaDate() { + return javaFormatter.parse("1234567890"); } @Benchmark - public void parseJodaDate() { - jodaFormatter.parse("1234567890"); + public TemporalAccessor parseJodaDate() { + return jodaFormatter.parse("1234567890"); } } diff --git a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java index ddc16d0656b24..d3bf5eb2a641c 100644 --- a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java +++ b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java @@ -1452,7 +1452,7 @@ public static DateFormatter forPattern(String input) { static JavaDateFormatter merge(String pattern, List formatters) { assert formatters.size() > 0; - List dateTimeFormatters = new ArrayList<>(); + List dateTimeFormatters = new ArrayList<>(formatters.size()); DateTimeFormatter printer = null; for (DateFormatter formatter : formatters) { assert formatter instanceof JavaDateFormatter; @@ -1464,7 +1464,7 @@ static JavaDateFormatter merge(String pattern, List formatters) { dateTimeFormatters.add(dateTimeFormatter); } - return new JavaDateFormatter(pattern, printer, dateTimeFormatters.toArray(new DateTimeFormatter[]{})); + return new JavaDateFormatter(pattern, printer, dateTimeFormatters.toArray(new DateTimeFormatter[0])); } private static final ZonedDateTime EPOCH_ZONED_DATE_TIME = Instant.EPOCH.atZone(ZoneOffset.UTC);