Added performance improvement for datetime field parsing

This adds caching of formatters in case of no explicit format specified for the datetime field in mapping. This also adds `strict_date_time_no_millis` as additional formatter in default date time formats Signed-off-by: Prabhat Sharma <[email protected]>
CaptainDredge · Oct 3, 2023 · 66bad21 · 66bad21
1 parent 28724ce
commit 66bad21
Show file tree

Hide file tree

Showing 13 changed files with 172 additions and 21 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -49,6 +49,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Change http code on create index API with bad input raising NotXContentException from 500 to 400 ([#4773](https://github.com/opensearch-project/OpenSearch/pull/4773))
 - Improve summary error message for invalid setting updates ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792))
 - Return 409 Conflict HTTP status instead of 503 on failure to concurrently execute snapshots ([#8986](https://github.com/opensearch-project/OpenSearch/pull/5855))
+- Performance improvement for Datetime field caching ([#4558](https://github.com/opensearch-project/OpenSearch/issues/4558))
 
 ### Deprecated
 

diff --git a/distribution/src/config/opensearch.yml b/distribution/src/config/opensearch.yml
@@ -121,3 +121,9 @@ ${path.logs}
 # index searcher threadpool.
 #
 #opensearch.experimental.feature.concurrent_segment_search.enabled: false
+#
+#
+# Gates the optimization of datetime formatters caching along with change in default datetime formatter
+# Once there is no observed impact on performance, this feature flag can be removed.
+#
+#opensearch.experimental.optimization.datetime_formatter_caching.enabled: false
diff --git a/server/src/main/java/org/opensearch/common/joda/JodaDateFormatter.java b/server/src/main/java/org/opensearch/common/joda/JodaDateFormatter.java
@@ -125,6 +125,11 @@ public String pattern() {
         return pattern;
     }
 
+    @Override
+    public String printPattern() {
+        throw new UnsupportedOperationException("JodaDateFormatter does not have a print pattern");
+    }
+
     @Override
     public Locale locale() {
         return printer.getLocale();

diff --git a/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java b/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java
@@ -39,7 +39,8 @@ protected FeatureFlagSettings(
                 FeatureFlags.EXTENSIONS_SETTING,
                 FeatureFlags.IDENTITY_SETTING,
                 FeatureFlags.CONCURRENT_SEGMENT_SEARCH_SETTING,
-                FeatureFlags.TELEMETRY_SETTING
+                FeatureFlags.TELEMETRY_SETTING,
+                FeatureFlags.DATETIME_FORMATTER_CACHING_SETTING
             )
         )
     );

diff --git a/server/src/main/java/org/opensearch/common/time/DateFormatter.java b/server/src/main/java/org/opensearch/common/time/DateFormatter.java
@@ -126,6 +126,14 @@ default String formatJoda(DateTime dateTime) {
      */
     String pattern();
 
+    /**
+     * A name based format for this formatter. Can be one of the registered formatters like <code>epoch_millis</code> or
+     * a configured format like <code>HH:mm:ss</code>
+     *
+     * @return The name of this formatter
+     */
+    String printPattern();
+
     /**
      * Returns the configured locale of the date formatter
      *
@@ -147,7 +155,7 @@ default String formatJoda(DateTime dateTime) {
      */
     DateMathParser toDateMathParser();
 
-    static DateFormatter forPattern(String input) {
+    static DateFormatter forPattern(String input, String printPattern, Boolean canCacheFormatter) {
 
         if (Strings.hasLength(input) == false) {
             throw new IllegalArgumentException("No date pattern provided");
@@ -158,7 +166,28 @@ static DateFormatter forPattern(String input) {
         List<String> patterns = splitCombinedPatterns(format);
         List<DateFormatter> formatters = patterns.stream().map(DateFormatters::forPattern).collect(Collectors.toList());
 
-        return JavaDateFormatter.combined(input, formatters);
+        DateFormatter printFormatter = formatters.get(0);
+        if (Strings.hasLength(printPattern)) {
+            String printFormat = strip8Prefix(printPattern);
+            try {
+                printFormatter = DateFormatters.forPattern(printFormat);
+            } catch (IllegalArgumentException e) {
+                throw new IllegalArgumentException("Invalid print format: " + e.getMessage(), e);
+            }
+        }
+        return JavaDateFormatter.combined(input, formatters, printFormatter, canCacheFormatter);
+    }
+
+    static DateFormatter forPattern(String input) {
+        return forPattern(input, null, false);
+    }
+
+    static DateFormatter forPattern(String input, String printPattern) {
+        return forPattern(input, printPattern, false);
+    }
+
+    static DateFormatter forPattern(String input, Boolean canCacheFormatter) {
+        return forPattern(input, null, canCacheFormatter);
     }
 
     static String strip8Prefix(String input) {

diff --git a/server/src/main/java/org/opensearch/common/time/JavaDateFormatter.java b/server/src/main/java/org/opensearch/common/time/JavaDateFormatter.java
@@ -32,6 +32,7 @@
 
 package org.opensearch.common.time;
 
+import org.opensearch.common.util.FeatureFlags;
 import org.opensearch.core.common.Strings;
 
 import java.text.ParsePosition;
@@ -67,9 +68,11 @@ class JavaDateFormatter implements DateFormatter {
     }
 
     private final String format;
+    private final String printFormat;
     private final DateTimeFormatter printer;
     private final List<DateTimeFormatter> parsers;
     private final JavaDateFormatter roundupParser;
+    private final Boolean canCacheLastParsedFormatter;
 
     /**
      * A round up formatter
@@ -93,8 +96,18 @@ JavaDateFormatter getRoundupParser() {
     }
 
     // named formatters use default roundUpParser
+    JavaDateFormatter(
+        String format,
+        String printFormat,
+        DateTimeFormatter printer,
+        Boolean canCacheLastParsedFormatter,
+        DateTimeFormatter... parsers
+    ) {
+        this(format, printFormat, printer, ROUND_UP_BASE_FIELDS, canCacheLastParsedFormatter, parsers);
+    }
+
     JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeFormatter... parsers) {
-        this(format, printer, ROUND_UP_BASE_FIELDS, parsers);
+        this(format, format, printer, false, parsers);
     }
 
     private static final BiConsumer<DateTimeFormatterBuilder, DateTimeFormatter> ROUND_UP_BASE_FIELDS = (builder, parser) -> {
@@ -111,8 +124,10 @@ JavaDateFormatter getRoundupParser() {
     // subclasses override roundUpParser
     JavaDateFormatter(
         String format,
+        String printFormat,
         DateTimeFormatter printer,
         BiConsumer<DateTimeFormatterBuilder, DateTimeFormatter> roundupParserConsumer,
+        Boolean canCacheLastParsedFormatter,
         DateTimeFormatter... parsers
     ) {
         if (printer == null) {
@@ -128,6 +143,8 @@ JavaDateFormatter getRoundupParser() {
         }
         this.printer = printer;
         this.format = format;
+        this.printFormat = printFormat;
+        this.canCacheLastParsedFormatter = canCacheLastParsedFormatter;
 
         if (parsers.length == 0) {
             this.parsers = Collections.singletonList(printer);
@@ -138,6 +155,15 @@ JavaDateFormatter getRoundupParser() {
         this.roundupParser = new RoundUpFormatter(format, roundUp);
     }
 
+    JavaDateFormatter(
+        String format,
+        DateTimeFormatter printer,
+        BiConsumer<DateTimeFormatterBuilder, DateTimeFormatter> roundupParserConsumer,
+        DateTimeFormatter... parsers
+    ) {
+        this(format, format, printer, roundupParserConsumer, false, parsers);
+    }
+
     /**
      * This is when the RoundUp Formatters are created. In further merges (with ||) it will only append them to a list.
      * || is not expected to be provided as format when a RoundUp formatter is created. It will be splitted before in
@@ -164,36 +190,54 @@ private List<DateTimeFormatter> createRoundUpParser(
         return null;
     }
 
-    public static DateFormatter combined(String input, List<DateFormatter> formatters) {
+    public static DateFormatter combined(
+        String input,
+        List<DateFormatter> formatters,
+        DateFormatter printFormatter,
+        Boolean canCacheLastParsedFormatter
+    ) {
         assert formatters.size() > 0;
+        assert printFormatter != null;
 
         List<DateTimeFormatter> parsers = new ArrayList<>(formatters.size());
         List<DateTimeFormatter> roundUpParsers = new ArrayList<>(formatters.size());
 
-        DateTimeFormatter printer = null;
+        assert printFormatter instanceof JavaDateFormatter;
+        JavaDateFormatter javaPrintFormatter = (JavaDateFormatter) printFormatter;
+        DateTimeFormatter printer = javaPrintFormatter.getPrinter();
         for (DateFormatter formatter : formatters) {
             assert formatter instanceof JavaDateFormatter;
             JavaDateFormatter javaDateFormatter = (JavaDateFormatter) formatter;
-            if (printer == null) {
-                printer = javaDateFormatter.getPrinter();
-            }
             parsers.addAll(javaDateFormatter.getParsers());
             roundUpParsers.addAll(javaDateFormatter.getRoundupParser().getParsers());
         }
 
-        return new JavaDateFormatter(input, printer, roundUpParsers, parsers);
+        return new JavaDateFormatter(input, javaPrintFormatter.format, printer, roundUpParsers, parsers, canCacheLastParsedFormatter);
     }
 
     private JavaDateFormatter(
         String format,
+        String printFormat,
         DateTimeFormatter printer,
         List<DateTimeFormatter> roundUpParsers,
-        List<DateTimeFormatter> parsers
+        List<DateTimeFormatter> parsers,
+        Boolean canCacheLastParsedFormatter
     ) {
         this.format = format;
+        this.printFormat = printFormat;
         this.printer = printer;
         this.roundupParser = roundUpParsers != null ? new RoundUpFormatter(format, roundUpParsers) : null;
         this.parsers = parsers;
+        this.canCacheLastParsedFormatter = canCacheLastParsedFormatter;
+    }
+
+    private JavaDateFormatter(
+        String format,
+        DateTimeFormatter printer,
+        List<DateTimeFormatter> roundUpParsers,
+        List<DateTimeFormatter> parsers
+    ) {
+        this(format, format, printer, roundUpParsers, parsers, false);
     }
 
     JavaDateFormatter getRoundupParser() {
@@ -237,6 +281,12 @@ private TemporalAccessor doParse(String input) {
                 ParsePosition pos = new ParsePosition(0);
                 Object object = formatter.toFormat().parseObject(input, pos);
                 if (parsingSucceeded(object, input, pos)) {
+                    if (FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING)
+                        && canCacheLastParsedFormatter
+                        && formatter != parsers.get(0)) {
+                        parsers.remove(formatter);
+                        parsers.add(0, formatter);
+                    }
                     return (TemporalAccessor) object;
                 }
             }
@@ -260,7 +310,7 @@ public DateFormatter withZone(ZoneId zoneId) {
             .stream()
             .map(p -> p.withZone(zoneId))
             .collect(Collectors.toList());
-        return new JavaDateFormatter(format, printer.withZone(zoneId), roundUpParsers, parsers);
+        return new JavaDateFormatter(format, printFormat, printer.withZone(zoneId), roundUpParsers, parsers, canCacheLastParsedFormatter);
     }
 
     @Override
@@ -274,7 +324,7 @@ public DateFormatter withLocale(Locale locale) {
             .stream()
             .map(p -> p.withLocale(locale))
             .collect(Collectors.toList());
-        return new JavaDateFormatter(format, printer.withLocale(locale), roundUpParsers, parsers);
+        return new JavaDateFormatter(format, printFormat, printer.withLocale(locale), roundUpParsers, parsers, canCacheLastParsedFormatter);
     }
 
     @Override
@@ -287,6 +337,11 @@ public String pattern() {
         return format;
     }
 
+    @Override
+    public String printPattern() {
+        return printFormat;
+    }
+
     @Override
     public Locale locale() {
         return this.printer.getLocale();

diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java
@@ -55,6 +55,11 @@ public class FeatureFlags {
      */
     public static final String TELEMETRY = "opensearch.experimental.feature.telemetry.enabled";
 
+    /**
+     * Gates the optimization of datetime formatters caching along with change in default datetime formatter.
+     */
+    public static final String DATETIME_FORMATTER_CACHING = "opensearch.experimental.optimization.datetime_formatter_caching.enabled";
+
     /**
      * Should store the settings from opensearch.yml.
      */
@@ -100,4 +105,10 @@ public static boolean isEnabled(String featureFlagName) {
         false,
         Property.NodeScope
     );
+
+    public static final Setting<Boolean> DATETIME_FORMATTER_CACHING_SETTING = Setting.boolSetting(
+        DATETIME_FORMATTER_CACHING,
+        false,
+        Property.NodeScope
+    );
 }
diff --git a/server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java
@@ -91,7 +91,15 @@ public final class DateFieldMapper extends ParametrizedFieldMapper {
 
     public static final String CONTENT_TYPE = "date";
     public static final String DATE_NANOS_CONTENT_TYPE = "date_nanos";
-    public static final DateFormatter DEFAULT_DATE_TIME_FORMATTER = DateFormatter.forPattern("strict_date_optional_time||epoch_millis");
+    @Deprecated
+    public static final DateFormatter LEGACY_DEFAULT_DATE_TIME_FORMATTER = DateFormatter.forPattern(
+        // TODO remove in 3.0 after backporting
+        "strict_date_optional_time||epoch_millis"
+    );
+    public static final DateFormatter DEFAULT_DATE_TIME_FORMATTER = DateFormatter.forPattern(
+        "strict_date_time_no_millis||strict_date_optional_time||epoch_millis",
+        "strict_date_optional_time"
+    );
 
     /**
      * Resolution of the date time
@@ -225,6 +233,12 @@ public static class Builder extends ParametrizedFieldMapper.Builder {
             m -> toType(m).format,
             DEFAULT_DATE_TIME_FORMATTER.pattern()
         );
+        private final Parameter<String> printFormat = Parameter.stringParam(
+            "print_format",
+            false,
+            m -> toType(m).printFormat,
+            DEFAULT_DATE_TIME_FORMATTER.printPattern()
+        ).acceptsNull();
         private final Parameter<Locale> locale = new Parameter<>(
             "locale",
             false,
@@ -253,21 +267,26 @@ public Builder(
             this.ignoreMalformed = Parameter.boolParam("ignore_malformed", true, m -> toType(m).ignoreMalformed, ignoreMalformedByDefault);
             if (dateFormatter != null) {
                 this.format.setValue(dateFormatter.pattern());
+                this.printFormat.setValue(dateFormatter.printPattern());
                 this.locale.setValue(dateFormatter.locale());
             }
         }
 
         private DateFormatter buildFormatter() {
             try {
-                return DateFormatter.forPattern(format.getValue()).withLocale(locale.getValue());
+                if (format.isConfigured() && !printFormat.isConfigured()) {
+                    return DateFormatter.forPattern(format.getValue(), null, !format.isConfigured()).withLocale(locale.getValue());
+                }
+                return DateFormatter.forPattern(format.getValue(), printFormat.getValue(), !format.isConfigured())
+                    .withLocale(locale.getValue());
             } catch (IllegalArgumentException e) {
                 throw new IllegalArgumentException("Error parsing [format] on field [" + name() + "]: " + e.getMessage(), e);
             }
         }
 
         @Override
         protected List<Parameter<?>> getParameters() {
-            return Arrays.asList(index, docValues, store, format, locale, nullValue, ignoreMalformed, boost, meta);
+            return Arrays.asList(index, docValues, store, format, printFormat, locale, nullValue, ignoreMalformed, boost, meta);
         }
 
         private Long parseNullValue(DateFieldType fieldType) {
@@ -610,6 +629,7 @@ public DocValueFormat docValueFormat(@Nullable String format, ZoneId timeZone) {
     private final boolean hasDocValues;
     private final Locale locale;
     private final String format;
+    private final String printFormat;
     private final boolean ignoreMalformed;
     private final Long nullValue;
     private final String nullValueAsString;
@@ -633,6 +653,7 @@ private DateFieldMapper(
         this.hasDocValues = builder.docValues.getValue();
         this.locale = builder.locale.getValue();
         this.format = builder.format.getValue();
+        this.printFormat = builder.printFormat.getValue();
         this.ignoreMalformed = builder.ignoreMalformed.getValue();
         this.nullValueAsString = builder.nullValue.getValue();
         this.nullValue = nullValue;