Complete fix and update tests.

Signed-off-by: Yury-Fridlyand <[email protected]>

Author: Yury-Fridlyand

integ-test/src/test/java/org/opensearch/sql/sql/DateTimeFormatsIT.java

@@ -92,16 +92,18 @@ public void testCustomFormats2() {
   @Test
   @SneakyThrows
   public void testIncompleteFormats() {
-    String query = String.format("SELECT incomplete_1, incomplete_2, incorrect"
-        + " FROM %s", TEST_INDEX_DATE_FORMATS);
+    String query = String.format("SELECT incomplete_1, incomplete_2, incorrect,"
+        + "incomplete_custom_time, incomplete_custom_date FROM %s", TEST_INDEX_DATE_FORMATS);
     JSONObject result = executeQuery(query);
     verifySchema(result,
         schema("incomplete_1", null, "timestamp"),
-        schema("incomplete_2", null, "timestamp"),
-        schema("incorrect", null, "timestamp"));
+        schema("incomplete_2", null, "date"),
+        schema("incorrect", null, "timestamp"),
+        schema("incomplete_custom_time", null, "time"),
+        schema("incomplete_custom_date", null, "date"));
     verifyDataRows(result,
-        rows("1984-01-01 00:00:00", null, null),
-        rows("2012-01-01 00:00:00", null, null));
+        rows("1984-01-01 00:00:00", null, null, "10:00:00", "1999-01-01"),
+        rows("2012-01-01 00:00:00", null, null, "20:00:00", "3021-01-01"));
   }
 
   @Test

opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java

@@ -11,14 +11,11 @@
 import static org.opensearch.sql.data.type.ExprCoreType.TIME;
 import static org.opensearch.sql.data.type.ExprCoreType.TIMESTAMP;
 
-import java.time.LocalDateTime;
-import java.time.temporal.TemporalAccessor;
 import java.util.List;
 import java.util.Objects;
 import java.util.stream.Collectors;
 import lombok.EqualsAndHashCode;
 import org.opensearch.common.time.DateFormatter;
-import org.opensearch.common.time.DateFormatters;
 import org.opensearch.common.time.FormatNames;
 import org.opensearch.sql.data.type.ExprCoreType;
 import org.opensearch.sql.data.type.ExprType;
@@ -31,13 +28,13 @@ public class OpenSearchDateType extends OpenSearchDataType {
 
   private static final OpenSearchDateType instance = new OpenSearchDateType();
 
-  // numeric formats which support full datetime
+  /** Numeric formats which support full datetime. */
   public static final List<FormatNames> SUPPORTED_NAMED_NUMERIC_FORMATS = List.of(
       FormatNames.EPOCH_MILLIS,
       FormatNames.EPOCH_SECOND
   );
 
-  // list of named formats which support full datetime
+  /** List of named formats which support full datetime. */
   public static final List<FormatNames> SUPPORTED_NAMED_DATETIME_FORMATS = List.of(
       FormatNames.ISO8601,
       FormatNames.BASIC_DATE_TIME,
@@ -78,7 +75,7 @@ public class OpenSearchDateType extends OpenSearchDataType {
       FormatNames.STRICT_WEEK_DATE_TIME_NO_MILLIS
   );
 
-  // list of named formats that only support year/month/day
+  /** List of named formats that only support year/month/day. */
   public static final List<FormatNames> SUPPORTED_NAMED_DATE_FORMATS = List.of(
       FormatNames.BASIC_DATE,
       FormatNames.BASIC_ORDINAL_DATE,
@@ -94,8 +91,8 @@ public class OpenSearchDateType extends OpenSearchDataType {
       FormatNames.STRICT_WEEKYEAR_WEEK_DAY
   );
 
-  // list of named formats which produce incomplete date,
-  // e.g. 1 or 2 are missing from tuple year/month/day
+  /** list of named formats which produce incomplete date,
+   * e.g. 1 or 2 are missing from tuple year/month/day. */
   public static final List<FormatNames> SUPPORTED_NAMED_INCOMPLETE_DATE_FORMATS = List.of(
       FormatNames.YEAR_MONTH,
       FormatNames.STRICT_YEAR_MONTH,
@@ -108,7 +105,7 @@ public class OpenSearchDateType extends OpenSearchDataType {
       FormatNames.STRICT_WEEKYEAR
   );
 
-  // list of named formats that only support hour/minute/second
+  /** List of named formats that only support hour/minute/second. */
   public static final List<FormatNames> SUPPORTED_NAMED_TIME_FORMATS = List.of(
       FormatNames.BASIC_TIME,
       FormatNames.BASIC_TIME_NO_MILLIS,
@@ -134,6 +131,11 @@ public class OpenSearchDateType extends OpenSearchDataType {
       FormatNames.STRICT_T_TIME_NO_MILLIS
   );
 
+  /** Formatter symbols which used to format time or date correspondingly.
+   * {@link java.time.format.DateTimeFormatter}. */
+  private static final String CUSTOM_FORMAT_TIME_SYMBOLS = "nNASsmHkKha";
+  private static final String CUSTOM_FORMAT_DATE_SYMBOLS = "FecEWwYqQgdMLDyuG";
+
   @EqualsAndHashCode.Exclude
   String formatString;
 
@@ -179,7 +181,6 @@ public List<DateFormatter> getAllNamedFormatters() {
 
   /**
    * Retrieves a list of numeric formatters that format for dates.
-   *
    * @return a list of DateFormatters that can be used to parse a Date.
    */
   public List<DateFormatter> getNumericNamedFormatters() {
@@ -191,6 +192,26 @@ public List<DateFormatter> getNumericNamedFormatters() {
         .map(DateFormatter::forPattern).collect(Collectors.toList());
   }
 
+  /**
+   * Retrieves a list of custom formats defined by the user.
+   * @return a list of formats as strings that can be used to parse a Date/Time/Timestamp.
+   */
+  public List<String> getAllCustomFormats() {
+    return getFormatList().stream()
+        .filter(format -> FormatNames.forName(format) == null)
+        .map(format -> {
+          try {
+            DateFormatter.forPattern(format);
+            return format;
+          } catch (Exception ignored) {
+            // parsing failed
+            return null;
+          }
+        })
+        .filter(Objects::nonNull)
+        .collect(Collectors.toList());
+  }
+
   /**
    * Retrieves a list of custom formatters defined by the user.
    * @return a list of DateFormatters that can be used to parse a Date/Time/Timestamp.
@@ -212,7 +233,6 @@ public List<DateFormatter> getAllCustomFormatters() {
 
   /**
    * Retrieves a list of named formatters that format for dates.
-   *
    * @return a list of DateFormatters that can be used to parse a Date.
    */
   public List<DateFormatter> getDateNamedFormatters() {
@@ -226,7 +246,6 @@ public List<DateFormatter> getDateNamedFormatters() {
 
   /**
    * Retrieves a list of named formatters that format for Times.
-   *
    * @return a list of DateFormatters that can be used to parse a Time.
    */
   public List<DateFormatter> getTimeNamedFormatters() {
@@ -240,7 +259,6 @@ public List<DateFormatter> getTimeNamedFormatters() {
 
   /**
    * Retrieves a list of named formatters that format for DateTimes.
-   *
    * @return a list of DateFormatters that can be used to parse a DateTime.
    */
   public List<DateFormatter> getDateTimeNamedFormatters() {
@@ -252,39 +270,26 @@ public List<DateFormatter> getDateTimeNamedFormatters() {
         .map(DateFormatter::forPattern).collect(Collectors.toList());
   }
 
-  private ExprCoreType getExprTypeFromCustomFormats(List<DateFormatter> formats) {
+  private ExprCoreType getExprTypeFromCustomFormats(List<String> formats) {
     boolean isDate = false;
     boolean isTime = false;
 
-    LocalDateTime sampleDateTime = LocalDateTime.now();
-    // Unfortunately, there is no public API to get info from the formatter object,
-    // whether it parses date or time or datetime. The workaround is:
-    // Converting a sample DateTime object by each formatter to string and back.
-    // Double-converted sample will be also DateTime, but if formatter parses
-    // time part only, date part would be lost. And vice versa.
-    // This trick allows us to get matching data type for every custom formatter.
-    // Unfortunately, this involves parsing a string, once per query, per column, per format.
-    // Overhead performance is equal to parsing extra row of result set (extra doc).
-    // Could be cached in scope of #1783 https://github.com/opensearch-project/sql/issues/1783.
-
     for (var format : formats) {
-      TemporalAccessor ta = format.parse(format.format(sampleDateTime));
-      LocalDateTime parsedSample = sampleDateTime;
-      try {
-        // TODO do we need withZoneSameInstant or withZoneSameLocal?
-        parsedSample = DateFormatters.from(ta).toLocalDateTime();
-      } catch (Exception ignored) {
-        // Can't convert to a DateTime - format does not represent a complete date or time
-        continue;
+      if (!isTime) {
+        for (var symbol : CUSTOM_FORMAT_TIME_SYMBOLS.toCharArray()) {
+          if (format.contains(String.valueOf(symbol))) {
+            isTime = true;
+            break;
+          }
+        }
       }
-
       if (!isDate) {
-        isDate = parsedSample.toLocalDate().equals(sampleDateTime.toLocalDate());
-      }
-      if (!isTime) {
-        // Second and Second fraction part are optional and may miss in some formats, trim it.
-        isTime = parsedSample.toLocalTime().withSecond(0).withNano(0).equals(
-            sampleDateTime.toLocalTime().withSecond(0).withNano(0));
+        for (var symbol : CUSTOM_FORMAT_DATE_SYMBOLS.toCharArray()) {
+          if (format.contains(String.valueOf(symbol))) {
+            isDate = true;
+            break;
+          }
+        }
       }
       if (isDate && isTime) {
         return TIMESTAMP;
@@ -298,7 +303,7 @@ private ExprCoreType getExprTypeFromCustomFormats(List<DateFormatter> formats) {
       return TIME;
     }
 
-    // Incomplete formats: can't be converted to DATE nor TIME, for example `year` (year only)
+    // Incomplete or incorrect formats: can't be converted to DATE nor TIME, for example `year`
     return TIMESTAMP;
   }
 
@@ -316,7 +321,7 @@ private ExprCoreType getExprTypeFromFormatString(String formatString) {
       return TIMESTAMP;
     }
 
-    List<DateFormatter> customFormatters = getAllCustomFormatters();
+    List<String> customFormatters = getAllCustomFormats();
     if (!customFormatters.isEmpty()) {
       ExprCoreType customFormatType = getExprTypeFromCustomFormats(customFormatters);
       ExprCoreType combinedByDefaultFormats = customFormatType;

opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java

@@ -55,6 +55,7 @@
 import org.opensearch.sql.data.model.ExprTimestampValue;
 import org.opensearch.sql.data.model.ExprTupleValue;
 import org.opensearch.sql.data.model.ExprValue;
+import org.opensearch.sql.data.type.ExprCoreType;
 import org.opensearch.sql.data.type.ExprType;
 import org.opensearch.sql.opensearch.data.type.OpenSearchDataType;
 import org.opensearch.sql.opensearch.data.type.OpenSearchDateType;
@@ -226,8 +227,7 @@ private ExprValue parseTimestampString(String value, OpenSearchDateType dateType
     try {
       parsed = DateFormatters.from(DATE_TIME_FORMATTER.parse(value)).toInstant();
       return new ExprTimestampValue(parsed);
-    } catch (DateTimeParseException e) {
-      // ignored
+    } catch (DateTimeParseException ignored) {
     }
 
     // otherwise, throw an error that no formatters worked
@@ -254,7 +254,7 @@ private ExprValue parseTimeString(String value, OpenSearchDateType dateType) {
         ZonedDateTime zonedDateTime = DateFormatters.from(accessor);
         return new ExprTimeValue(
             zonedDateTime.withZoneSameLocal(UTC_ZONE_ID).toLocalTime());
-      } catch (IllegalArgumentException  ignored) {
+      } catch (IllegalArgumentException ignored) {
         // nothing to do, try another format
       }
     }
@@ -263,8 +263,7 @@ private ExprValue parseTimeString(String value, OpenSearchDateType dateType) {
     try {
       return new ExprTimeValue(
           DateFormatters.from(STRICT_HOUR_MINUTE_SECOND_FORMATTER.parse(value)).toLocalTime());
-    } catch (DateTimeParseException e) {
-      // ignored
+    } catch (DateTimeParseException ignored) {
     }
 
     throw new IllegalArgumentException("Construct ExprTimeValue from \"" + value
@@ -289,7 +288,7 @@ private ExprValue parseDateString(String value, OpenSearchDateType dateType) {
         // return the first matching formatter as a date without timezone
         return new ExprDateValue(
             zonedDateTime.withZoneSameLocal(UTC_ZONE_ID).toLocalDate());
-      } catch (IllegalArgumentException  ignored) {
+      } catch (IllegalArgumentException ignored) {
         // nothing to do, try another format
       }
     }
@@ -298,8 +297,7 @@ private ExprValue parseDateString(String value, OpenSearchDateType dateType) {
     try {
       return new ExprDateValue(
           DateFormatters.from(STRICT_YEAR_MONTH_DAY_FORMATTER.parse(value)).toLocalDate());
-    } catch (DateTimeParseException e) {
-      // ignored
+    } catch (DateTimeParseException ignored) {
     }
 
     throw new IllegalArgumentException("Construct ExprDateValue from \"" + value
@@ -310,7 +308,7 @@ private ExprValue createOpenSearchDateType(Content value, ExprType type) {
     OpenSearchDateType dt = (OpenSearchDateType) type;
     ExprType returnFormat = dt.getExprType();
 
-    if (value.isNumber()) {
+    if (value.isNumber()) { // isNumber
       var numFormatters = dt.getNumericNamedFormatters();
       if (numFormatters.size() > 0) {
         long epochMillis = 0;
@@ -321,16 +319,14 @@ private ExprValue createOpenSearchDateType(Content value, ExprType type) {
         } else /* EPOCH_SECOND */ {
           epochMillis = value.longValue() * 1000;
         }
-        Instant instant = Instant.ofEpochMilli(epochMillis);
-        if (returnFormat == TIME) {
-          return new ExprTimeValue(LocalTime.from(instant.atZone(UTC_ZONE_ID)));
-        }
-        if (returnFormat == DATE) {
-          return new ExprDateValue(LocalDate.ofInstant(instant, UTC_ZONE_ID));
-        }
-        return new ExprTimestampValue(instant);
+        return new ExprTimestampValue(Instant.ofEpochMilli(epochMillis));
       } else {
-        return parseTimestampString(value.stringValue(), dt);
+        // custom format
+        switch ((ExprCoreType) dt.getExprType()) {
+          case TIME: return parseTimeString(value.stringValue(), dt);
+          case DATE: return parseDateString(value.stringValue(), dt);
+          default: return parseTimestampString(value.stringValue(), dt);
+        }
       }
     }