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]>
opensearch-project · Aug 28, 2023 · b2901c7 · b2901c7
1 parent 8cfde6c
commit b2901c7
Show file tree

Hide file tree

Showing 7 changed files with 66 additions and 15 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -158,6 +158,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Removing the vec file extension from INDEX_STORE_HYBRID_NIO_EXTENSIONS, to ensure the no performance degradation for vector search via Lucene Engine.([#9528](https://github.com/opensearch-project/OpenSearch/pull/9528)))
 - Separate request-based and settings-based concurrent segment search controls and introduce AggregatorFactory method to determine concurrent search support ([#9469](https://github.com/opensearch-project/OpenSearch/pull/9469))
 - [Remote Store] Rate limiter integration for remote store uploads and downloads([#9448](https://github.com/opensearch-project/OpenSearch/pull/9448/))
+- 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
@@ -129,3 +129,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/settings/FeatureFlagSettings.java b/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java
@@ -40,7 +40,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
@@ -147,7 +147,7 @@ default String formatJoda(DateTime dateTime) {
      */
     DateMathParser toDateMathParser();
 
-    static DateFormatter forPattern(String input) {
+    static DateFormatter forPattern(String input, Boolean canCacheFormatter) {
 
         if (Strings.hasLength(input) == false) {
             throw new IllegalArgumentException("No date pattern provided");
@@ -158,7 +158,11 @@ 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);
+        return JavaDateFormatter.combined(input, formatters, canCacheFormatter);
+    }
+
+    static DateFormatter forPattern(String input) {
+        return forPattern(input, false);
     }
 
     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;
@@ -70,6 +71,7 @@ class JavaDateFormatter implements DateFormatter {
     private final DateTimeFormatter printer;
     private final List<DateTimeFormatter> parsers;
     private final JavaDateFormatter roundupParser;
+    private final Boolean canCacheLastParsedFormatter;
 
     /**
      * A round up formatter
@@ -78,8 +80,8 @@ class JavaDateFormatter implements DateFormatter {
      */
     static class RoundUpFormatter extends JavaDateFormatter {
 
-        RoundUpFormatter(String format, List<DateTimeFormatter> roundUpParsers) {
-            super(format, firstFrom(roundUpParsers), null, roundUpParsers);
+        RoundUpFormatter(String format, List<DateTimeFormatter> roundUpParsers, Boolean canCacheLastParsedFormatter) {
+            super(format, firstFrom(roundUpParsers), null, roundUpParsers, canCacheLastParsedFormatter);
         }
 
         private static DateTimeFormatter firstFrom(List<DateTimeFormatter> roundUpParsers) {
@@ -93,8 +95,12 @@ JavaDateFormatter getRoundupParser() {
     }
 
     // named formatters use default roundUpParser
+    JavaDateFormatter(String format, DateTimeFormatter printer, Boolean canCacheLastParsedFormatter, DateTimeFormatter... parsers) {
+        this(format, printer, ROUND_UP_BASE_FIELDS, canCacheLastParsedFormatter, parsers);
+    }
+
     JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeFormatter... parsers) {
-        this(format, printer, ROUND_UP_BASE_FIELDS, parsers);
+        this(format, printer, false, parsers);
     }
 
     private static final BiConsumer<DateTimeFormatterBuilder, DateTimeFormatter> ROUND_UP_BASE_FIELDS = (builder, parser) -> {
@@ -113,6 +119,7 @@ JavaDateFormatter getRoundupParser() {
         String format,
         DateTimeFormatter printer,
         BiConsumer<DateTimeFormatterBuilder, DateTimeFormatter> roundupParserConsumer,
+        Boolean canCacheLastParsedFormatter,
         DateTimeFormatter... parsers
     ) {
         if (printer == null) {
@@ -128,14 +135,24 @@ JavaDateFormatter getRoundupParser() {
         }
         this.printer = printer;
         this.format = format;
+        this.canCacheLastParsedFormatter = canCacheLastParsedFormatter;
 
         if (parsers.length == 0) {
             this.parsers = Collections.singletonList(printer);
         } else {
             this.parsers = Arrays.asList(parsers);
         }
         List<DateTimeFormatter> roundUp = createRoundUpParser(format, roundupParserConsumer);
-        this.roundupParser = new RoundUpFormatter(format, roundUp);
+        this.roundupParser = new RoundUpFormatter(format, roundUp, canCacheLastParsedFormatter);
+    }
+
+    JavaDateFormatter(
+        String format,
+        DateTimeFormatter printer,
+        BiConsumer<DateTimeFormatterBuilder, DateTimeFormatter> roundupParserConsumer,
+        DateTimeFormatter... parsers
+    ) {
+        this(format, printer, roundupParserConsumer, false, parsers);
     }
 
     /**
@@ -164,7 +181,7 @@ private List<DateTimeFormatter> createRoundUpParser(
         return null;
     }
 
-    public static DateFormatter combined(String input, List<DateFormatter> formatters) {
+    public static DateFormatter combined(String input, List<DateFormatter> formatters, Boolean canCacheLastParsedFormatter) {
         assert formatters.size() > 0;
 
         List<DateTimeFormatter> parsers = new ArrayList<>(formatters.size());
@@ -181,19 +198,21 @@ public static DateFormatter combined(String input, List<DateFormatter> formatter
             roundUpParsers.addAll(javaDateFormatter.getRoundupParser().getParsers());
         }
 
-        return new JavaDateFormatter(input, printer, roundUpParsers, parsers);
+        return new JavaDateFormatter(input, printer, roundUpParsers, parsers, canCacheLastParsedFormatter);
     }
 
     private JavaDateFormatter(
         String format,
         DateTimeFormatter printer,
         List<DateTimeFormatter> roundUpParsers,
-        List<DateTimeFormatter> parsers
+        List<DateTimeFormatter> parsers,
+        Boolean canCacheLastParsedFormatter
     ) {
         this.format = format;
         this.printer = printer;
-        this.roundupParser = roundUpParsers != null ? new RoundUpFormatter(format, roundUpParsers) : null;
+        this.roundupParser = roundUpParsers != null ? new RoundUpFormatter(format, roundUpParsers, canCacheLastParsedFormatter) : null;
         this.parsers = parsers;
+        this.canCacheLastParsedFormatter = canCacheLastParsedFormatter;
     }
 
     JavaDateFormatter getRoundupParser() {
@@ -237,6 +256,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 +285,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, printer.withZone(zoneId), roundUpParsers, parsers, this.canCacheLastParsedFormatter);
     }
 
     @Override
@@ -274,7 +299,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, printer.withLocale(locale), roundUpParsers, parsers, this.canCacheLastParsedFormatter);
     }
 
     @Override

diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java
@@ -62,6 +62,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.
      */
@@ -109,4 +114,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,10 @@ 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");
+    public static final DateFormatter DEFAULT_DATE_TIME_FORMATTER = DateFormatter.forPattern(
+        "strict_date_time_no_millis||strict_date_optional_time||epoch_millis",
+        true
+    );
 
     /**
      * Resolution of the date time
@@ -259,7 +262,7 @@ public Builder(
 
         private DateFormatter buildFormatter() {
             try {
-                return DateFormatter.forPattern(format.getValue()).withLocale(locale.getValue());
+                return DateFormatter.forPattern(format.getValue(), !format.isConfigured()).withLocale(locale.getValue());
             } catch (IllegalArgumentException e) {
                 throw new IllegalArgumentException("Error parsing [format] on field [" + name() + "]: " + e.getMessage(), e);
             }