Date/Time parsing: Use java time API instead of exception handling

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("8year_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");
+    }
+}
+

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);
     }
 }

server/src/main/java/org/elasticsearch/common/time/DateFormatters.java

@@ -19,30 +19,26 @@
 
 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;
 import java.time.temporal.IsoFields;
 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;
@@ -122,29 +118,27 @@ 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()
+        .appendZoneOrOffsetId()
         .optionalEnd()
         .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,90 +1441,22 @@ public static DateFormatter forPattern(String input) {
         }
     }
 
-    static class MergedDateFormatter implements DateFormatter {
-
-        private final String pattern;
-        // package private for tests
-        final List<DateFormatter> formatters;
-        private final List<DateMathParser> dateMathParsers;
-
-        MergedDateFormatter(String pattern, List<DateFormatter> formatters) {
-            assert formatters.size() > 0;
-            this.pattern = pattern;
-            this.formatters = Collections.unmodifiableList(formatters);
-            this.dateMathParsers = formatters.stream().map(DateFormatter::toDateMathParser).collect(Collectors.toList());
-        }
-
-        @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);
-                    }
-                }
+    static JavaDateFormatter merge(String pattern, List<DateFormatter> formatters) {
+        assert formatters.size() > 0;
+
+        List<DateTimeFormatter> 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();
             }
-            throw failure;
+            dateTimeFormatters.add(dateTimeFormatter);
         }
 
-        @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, printer, dateTimeFormatters.toArray(new DateTimeFormatter[]{}));
     }
 
     private static final ZonedDateTime EPOCH_ZONED_DATE_TIME = Instant.EPOCH.atZone(ZoneOffset.UTC);

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,54 @@ 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;
     }
 
+    DateTimeFormatter getParser() {
+        return parser;
+    }
+
+    DateTimeFormatter getPrinter() {
+        return printer;
+    }
+
     @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 +126,7 @@ public String pattern() {
     JavaDateFormatter parseDefaulting(Map<TemporalField, Long> 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

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");