DATETIME_FORMATTER_CACHING_SETTING experimental feature should not de…

…fault to 'true' (#13532)

Signed-off-by: Andriy Redko <[email protected]>

CHANGELOG.md

@@ -97,6 +97,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Improve the error messages for _stats with closed indices ([#13012](https://github.com/opensearch-project/OpenSearch/pull/13012))
 - Ignore BaseRestHandler unconsumed content check as it's always consumed. ([#13290](https://github.com/opensearch-project/OpenSearch/pull/13290))
 - Fix mapper_parsing_exception when using flat_object fields with names longer than 11 characters ([#13259](https://github.com/opensearch-project/OpenSearch/pull/13259))
+- DATETIME_FORMATTER_CACHING_SETTING experimental feature should not default to 'true' ([#13532](https://github.com/opensearch-project/OpenSearch/pull/13532))
 
 ### Security
 

server/src/main/java/org/opensearch/common/util/FeatureFlags.java

@@ -81,7 +81,7 @@ public class FeatureFlags {
 
     public static final Setting<Boolean> DATETIME_FORMATTER_CACHING_SETTING = Setting.boolSetting(
         DATETIME_FORMATTER_CACHING,
-        true,
+        false,
         Property.NodeScope
     );
 

server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java

@@ -39,30 +39,24 @@ public void testNonBooleanFeatureFlag() {
         assertFalse(FeatureFlags.isEnabled(javaVersionProperty));
     }
 
-    public void testBooleanFeatureFlagWithDefaultSetToTrue() {
-        final String testFlag = DATETIME_FORMATTER_CACHING;
-        assertNotNull(testFlag);
-        assertTrue(FeatureFlags.isEnabled(testFlag));
-    }
-
     public void testBooleanFeatureFlagWithDefaultSetToFalse() {
         final String testFlag = IDENTITY;
         FeatureFlags.initializeFeatureFlags(Settings.EMPTY);
         assertNotNull(testFlag);
         assertFalse(FeatureFlags.isEnabled(testFlag));
     }
 
-    public void testBooleanFeatureFlagInitializedWithEmptySettingsAndDefaultSetToTrue() {
+    public void testBooleanFeatureFlagInitializedWithEmptySettingsAndDefaultSetToFalse() {
         final String testFlag = DATETIME_FORMATTER_CACHING;
         FeatureFlags.initializeFeatureFlags(Settings.EMPTY);
         assertNotNull(testFlag);
-        assertTrue(FeatureFlags.isEnabled(testFlag));
+        assertFalse(FeatureFlags.isEnabled(testFlag));
     }
 
     public void testInitializeFeatureFlagsWithExperimentalSettings() {
         FeatureFlags.initializeFeatureFlags(Settings.builder().put(IDENTITY, true).build());
         assertTrue(FeatureFlags.isEnabled(IDENTITY));
-        assertTrue(FeatureFlags.isEnabled(DATETIME_FORMATTER_CACHING));
+        assertFalse(FeatureFlags.isEnabled(DATETIME_FORMATTER_CACHING));
         assertFalse(FeatureFlags.isEnabled(EXTENSIONS));
         // reset FeatureFlags to defaults
         FeatureFlags.initializeFeatureFlags(Settings.EMPTY);

server/src/test/java/org/opensearch/index/mapper/DateFieldMapperTests.java

@@ -35,6 +35,7 @@
 import org.apache.lucene.index.DocValuesType;
 import org.apache.lucene.index.IndexableField;
 import org.opensearch.common.time.DateFormatter;
+import org.opensearch.common.util.FeatureFlags;
 import org.opensearch.core.xcontent.XContentBuilder;
 import org.opensearch.index.termvectors.TermVectorsService;
 import org.opensearch.search.DocValueFormat;
@@ -45,8 +46,10 @@
 import java.time.ZonedDateTime;
 import java.util.List;
 
+import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.notNullValue;
+import static org.junit.Assume.assumeThat;
 
 public class DateFieldMapperTests extends MapperTestCase {
 
@@ -146,7 +149,22 @@ public void testStore() throws Exception {
         assertEquals(1457654400000L, storedField.numericValue().longValue());
     }
 
+    public void testIgnoreMalformedLegacy() throws IOException {
+        assumeThat("Using legacy datetime format as default", FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING), is(false));
+        testIgnoreMalformedForValue(
+            "2016-03-99",
+            "failed to parse date field [2016-03-99] with format [strict_date_optional_time||epoch_millis]"
+        );
+        testIgnoreMalformedForValue("-2147483648", "Invalid value for Year (valid values -999999999 - 999999999): -2147483648");
+        testIgnoreMalformedForValue("-522000000", "long overflow");
+    }
+
     public void testIgnoreMalformed() throws IOException {
+        assumeThat(
+            "Using experimental datetime format as default",
+            FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING),
+            is(true)
+        );
         testIgnoreMalformedForValue(
             "2016-03-99",
             "failed to parse date field [2016-03-99] with format [strict_date_time_no_millis||strict_date_optional_time||epoch_millis]"

server/src/test/java/org/opensearch/index/mapper/RangeFieldMapperTests.java

@@ -37,6 +37,7 @@
 import org.apache.lucene.index.IndexableField;
 import org.opensearch.common.CheckedConsumer;
 import org.opensearch.common.network.InetAddresses;
+import org.opensearch.common.util.FeatureFlags;
 import org.opensearch.common.xcontent.XContentFactory;
 import org.opensearch.core.xcontent.ToXContent;
 import org.opensearch.core.xcontent.XContentBuilder;
@@ -51,8 +52,10 @@
 import static org.opensearch.index.query.RangeQueryBuilder.GT_FIELD;
 import static org.opensearch.index.query.RangeQueryBuilder.LTE_FIELD;
 import static org.opensearch.index.query.RangeQueryBuilder.LT_FIELD;
+import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.Matchers.anyOf;
 import static org.hamcrest.Matchers.containsString;
+import static org.junit.Assume.assumeThat;
 
 public class RangeFieldMapperTests extends AbstractNumericFieldMapperTestCase {
     private static final String FROM_DATE = "2016-10-31";
@@ -351,7 +354,30 @@ public void testIllegalArguments() throws Exception {
         assertThat(e.getMessage(), containsString("should not define a dateTimeFormatter"));
     }
 
+    public void testSerializeDefaultsLegacy() throws Exception {
+        assumeThat("Using legacy datetime format as default", FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING), is(false));
+
+        for (String type : types()) {
+            DocumentMapper docMapper = createDocumentMapper(fieldMapping(b -> b.field("type", type)));
+            RangeFieldMapper mapper = (RangeFieldMapper) docMapper.root().getMapper("field");
+            XContentBuilder builder = XContentFactory.jsonBuilder().startObject();
+            mapper.doXContentBody(builder, true, ToXContent.EMPTY_PARAMS);
+            String got = builder.endObject().toString();
+
+            // if type is date_range we check that the mapper contains the default format and locale
+            // otherwise it should not contain a locale or format
+            assertTrue(got, got.contains("\"format\":\"strict_date_optional_time||epoch_millis\"") == type.equals("date_range"));
+            assertTrue(got, got.contains("\"locale\":" + "\"" + Locale.ROOT + "\"") == type.equals("date_range"));
+        }
+    }
+
     public void testSerializeDefaults() throws Exception {
+        assumeThat(
+            "Using experimental datetime format as default",
+            FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING),
+            is(true)
+        );
+
         for (String type : types()) {
             DocumentMapper docMapper = createDocumentMapper(fieldMapping(b -> b.field("type", type)));
             RangeFieldMapper mapper = (RangeFieldMapper) docMapper.root().getMapper("field");

server/src/test/java/org/opensearch/index/mapper/RangeFieldTypeTests.java

@@ -51,6 +51,7 @@
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.time.DateFormatter;
 import org.opensearch.common.util.BigArrays;
+import org.opensearch.common.util.FeatureFlags;
 import org.opensearch.index.IndexSettings;
 import org.opensearch.index.mapper.DateFieldMapper.DateFieldType;
 import org.opensearch.index.mapper.RangeFieldMapper.RangeFieldType;
@@ -65,8 +66,10 @@
 import java.util.Collections;
 import java.util.Map;
 
+import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.junit.Assume.assumeThat;
 
 public class RangeFieldTypeTests extends FieldTypeTestCase {
     RangeType type;
@@ -249,7 +252,49 @@ private QueryShardContext createContext() {
         );
     }
 
+    public void testDateRangeQueryUsingMappingFormatLegacy() {
+        assumeThat("Using legacy datetime format as default", FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING), is(false));
+
+        QueryShardContext context = createContext();
+        RangeFieldType strict = new RangeFieldType("field", RangeFieldMapper.Defaults.DATE_FORMATTER);
+        // don't use DISJOINT here because it doesn't work on date fields which we want to compare bounds with
+        ShapeRelation relation = randomValueOtherThan(ShapeRelation.DISJOINT, () -> randomFrom(ShapeRelation.values()));
+
+        // dates will break the default format, month/day of month is turned around in the format
+        final String from = "2016-15-06T15:29:50+08:00";
+        final String to = "2016-16-06T15:29:50+08:00";
+
+        OpenSearchParseException ex = expectThrows(
+            OpenSearchParseException.class,
+            () -> strict.rangeQuery(from, to, true, true, relation, null, null, context)
+        );
+        assertThat(
+            ex.getMessage(),
+            containsString("failed to parse date field [2016-15-06T15:29:50+08:00] with format [strict_date_optional_time||epoch_millis]")
+        );
+
+        // setting mapping format which is compatible with those dates
+        final DateFormatter formatter = DateFormatter.forPattern("yyyy-dd-MM'T'HH:mm:ssZZZZZ");
+        assertEquals(1465975790000L, formatter.parseMillis(from));
+        assertEquals(1466062190000L, formatter.parseMillis(to));
+
+        RangeFieldType fieldType = new RangeFieldType("field", formatter);
+        final Query query = fieldType.rangeQuery(from, to, true, true, relation, null, fieldType.dateMathParser(), context);
+        assertEquals("field:<ranges:[1465975790000 : 1466062190999]>", ((IndexOrDocValuesQuery) query).getIndexQuery().toString());
+
+        // compare lower and upper bounds with what we would get on a `date` field
+        DateFieldType dateFieldType = new DateFieldType("field", DateFieldMapper.Resolution.MILLISECONDS, formatter);
+        final Query queryOnDateField = dateFieldType.rangeQuery(from, to, true, true, relation, null, fieldType.dateMathParser(), context);
+        assertEquals("field:[1465975790000 TO 1466062190999]", ((IndexOrDocValuesQuery) queryOnDateField).getIndexQuery().toString());
+    }
+
     public void testDateRangeQueryUsingMappingFormat() {
+        assumeThat(
+            "Using experimental datetime format as default",
+            FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING),
+            is(true)
+        );
+
         QueryShardContext context = createContext();
         RangeFieldType strict = new RangeFieldType("field", RangeFieldMapper.Defaults.DATE_FORMATTER);
         // don't use DISJOINT here because it doesn't work on date fields which we want to compare bounds with