Addressed PR comments

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
Bit-Quill · May 29, 2023 · 58a8f19 · 58a8f19
1 parent 2672a79
commit 58a8f19
Show file tree

Hide file tree

Showing 7 changed files with 41 additions and 38 deletions.
diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java
@@ -193,6 +193,10 @@ public static OpenSearchDataType of(ExprType type) {
     if (res != null) {
       return res;
     }
+    if (OpenSearchDateType.isDateTypeCompatible(type)) {
+      return OpenSearchDateType.of(type);
+    }
+
     return new OpenSearchDataType((ExprCoreType) type);
   }
 

diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java
@@ -8,6 +8,8 @@
 import static org.opensearch.common.time.DateFormatter.splitCombinedPatterns;
 import static org.opensearch.common.time.DateFormatter.strip8Prefix;
 import static org.opensearch.sql.data.type.ExprCoreType.DATE;
+import static org.opensearch.sql.data.type.ExprCoreType.TIME;
+import static org.opensearch.sql.data.type.ExprCoreType.TIMESTAMP;
 
 import java.util.List;
 import java.util.stream.Collectors;
@@ -25,8 +27,6 @@ public class OpenSearchDateType extends OpenSearchDataType {
 
   private static final OpenSearchDateType instance = new OpenSearchDateType();
 
-  private static final String FORMAT_DELIMITER = "\\|\\|";
-
   public static final List<FormatNames> SUPPORTED_NAMED_DATETIME_FORMATS = List.of(
       FormatNames.ISO8601,
       FormatNames.EPOCH_MILLIS,
@@ -129,14 +129,12 @@ private OpenSearchDateType() {
   }
 
   private OpenSearchDateType(ExprCoreType exprCoreType) {
-    super(MappingType.Date);
-    this.formatString = "";
+    this();
     this.exprCoreType = exprCoreType;
   }
 
   private OpenSearchDateType(ExprType exprType) {
-    super(MappingType.Date);
-    this.formatString = "";
+    this();
     this.exprCoreType = (ExprCoreType) exprType;
   }
 
@@ -158,7 +156,7 @@ private List<String> getFormatList() {
 
 
   /**
-   * Retrieves a list of named formatters defined by OpenSearch.
+   * Retrieves a list of named OpenSearch formatters given by user mapping.
    * @return a list of DateFormatters that can be used to parse a Date/Time/Timestamp.
    */
   public List<DateFormatter> getAllNamedFormatters() {
@@ -207,26 +205,26 @@ public List<DateFormatter> getTimeNamedFormatters() {
 
   private ExprCoreType getExprTypeFromFormatString(String formatString) {
     if (formatString.isEmpty()) {
-      // TODO: check the default formatter - and set it here instead of assuming that the default
-      // is always a timestamp
-      return ExprCoreType.TIMESTAMP;
+      // FOLLOW-UP: check the default formatter - and set it here instead
+      // of assuming that the default is always a timestamp
+      return TIMESTAMP;
     }
 
     List<DateFormatter> namedFormatters = getAllNamedFormatters();
 
     if (namedFormatters.isEmpty()) {
-      return ExprCoreType.TIMESTAMP;
+      return TIMESTAMP;
     }
 
     if (!getAllCustomFormatters().isEmpty()) {
-      // TODO: support custom format in <issue#>
-      return ExprCoreType.TIMESTAMP;
+      // FOLLOW-UP: support custom format in <issue#>
+      return TIMESTAMP;
     }
 
     // if there is nothing in the dateformatter that accepts a year/month/day, then
     // we can assume the type is strictly a Time object
     if (namedFormatters.size() == getTimeNamedFormatters().size()) {
-      return ExprCoreType.TIME;
+      return TIME;
     }
 
     // if there is nothing in the dateformatter that accepts a hour/minute/second, then
@@ -235,7 +233,8 @@ private ExprCoreType getExprTypeFromFormatString(String formatString) {
       return DATE;
     }
 
-    return ExprCoreType.TIMESTAMP;
+    // According to the user mapping, this field may contain a DATE or a TIME
+    return TIMESTAMP;
   }
 
   /**

diff --git a/...ch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java b/...ch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java
@@ -116,13 +116,13 @@ public void extendTypeMapping(Map<String, OpenSearchDataType> typeMapping) {
               (c, dt) -> ExprBooleanValue.of(c.booleanValue()))
           //Handles the creation of DATE, TIME & DATETIME
           .put(OpenSearchDateType.of(TIME),
-              (c, dt) -> createOpenSearchDateType(c, dt))
+              this::createOpenSearchDateType)
           .put(OpenSearchDateType.of(DATE),
-              (c, dt) -> createOpenSearchDateType(c, dt))
+              this::createOpenSearchDateType)
           .put(OpenSearchDateType.of(TIMESTAMP),
-              (c, dt) -> createOpenSearchDateType(c, dt))
+              this::createOpenSearchDateType)
           .put(OpenSearchDateType.of(DATETIME),
-              (c, dt) -> createOpenSearchDateType(c, dt))
+              this::createOpenSearchDateType)
           .put(OpenSearchDataType.of(OpenSearchDataType.MappingType.Ip),
               (c, dt) -> new OpenSearchExprIpValue(c.stringValue()))
           .put(OpenSearchDataType.of(OpenSearchDataType.MappingType.GeoPoint),
@@ -202,7 +202,7 @@ private Optional<ExprType> type(String field) {
   }
 
   /**
-   * return the first matching formatter as an Instant to UTF.
+   * Parses value with the first matching formatter as an Instant to UTF.
    *
    * @param value - timestamp as string
    * @param dateType - field type

diff --git a/...ava/org/opensearch/sql/opensearch/storage/script/aggregation/AggregationQueryBuilder.java b/...ava/org/opensearch/sql/opensearch/storage/script/aggregation/AggregationQueryBuilder.java
@@ -113,18 +113,10 @@ public Map<String, OpenSearchDataType> buildTypeMapping(
       List<NamedExpression> groupByList) {
     ImmutableMap.Builder<String, OpenSearchDataType> builder = new ImmutableMap.Builder<>();
     namedAggregatorList.forEach(agg -> {
-      if (OpenSearchDateType.isDateTypeCompatible(agg.type())) {
-        builder.put(agg.getName(), OpenSearchDateType.of(agg.type()));
-      } else {
-        builder.put(agg.getName(), OpenSearchDataType.of(agg.type()));
-      }
+      builder.put(agg.getName(), OpenSearchDataType.of(agg.type()));
     });
     groupByList.forEach(group -> {
-      if (OpenSearchDateType.isDateTypeCompatible(group.type())) {
-        builder.put(group.getNameOrAlias(), OpenSearchDateType.of(group.type()));
-      } else {
-        builder.put(group.getNameOrAlias(), OpenSearchDataType.of(group.type()));
-      }
+      builder.put(group.getNameOrAlias(), OpenSearchDataType.of(group.type()));
     });
     return builder.build();
   }

diff --git a/...rch/src/main/java/org/opensearch/sql/opensearch/storage/script/core/ExpressionScript.java b/...rch/src/main/java/org/opensearch/sql/opensearch/storage/script/core/ExpressionScript.java
@@ -114,9 +114,6 @@ public Object visitParse(ParseExpression node, Set<ReferenceExpression> context)
   private OpenSearchExprValueFactory buildValueFactory(Set<ReferenceExpression> fields) {
     Map<String, OpenSearchDataType> typeEnv = fields.stream().collect(toMap(
         ReferenceExpression::getAttr, e -> {
-          if (OpenSearchDateType.isDateTypeCompatible(e.type())) {
-            return OpenSearchDateType.of(e.type());
-          }
           return OpenSearchDataType.of(e.type());
         }));
     return new OpenSearchExprValueFactory(typeEnv);

diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeTest.java
@@ -137,11 +137,13 @@ public void of_MappingType(MappingType mappingType, String name, ExprType dataTy
   public void of_ExprCoreType(ExprCoreType coreType) {
     assumeFalse(coreType == UNKNOWN);
     var type = OpenSearchDataType.of(coreType);
-    assertAll(
-        () -> assertEquals(coreType.toString(), type.typeName()),
-        () -> assertEquals(coreType.toString(), type.legacyTypeName()),
-        () -> assertEquals(coreType, type.getExprType())
-    );
+    if (type instanceof OpenSearchDateType) {
+      assertEquals(coreType, type.getExprType());
+    } else {
+      assertEquals(coreType.toString(), type.typeName());
+      assertEquals(coreType.toString(), type.legacyTypeName());
+      assertEquals(coreType, type.getExprType());
+    }
   }
 
   @ParameterizedTest(name = "{0}")

diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateTypeTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateTypeTest.java
@@ -23,12 +23,14 @@
 import static org.opensearch.sql.opensearch.data.type.OpenSearchDateType.SUPPORTED_NAMED_DATETIME_FORMATS;
 import static org.opensearch.sql.opensearch.data.type.OpenSearchDateType.SUPPORTED_NAMED_DATE_FORMATS;
 import static org.opensearch.sql.opensearch.data.type.OpenSearchDateType.SUPPORTED_NAMED_TIME_FORMATS;
+import static org.opensearch.sql.opensearch.data.type.OpenSearchDateType.isDateTypeCompatible;
 
 import java.util.EnumSet;
 import org.junit.jupiter.api.DisplayNameGeneration;
 import org.junit.jupiter.api.DisplayNameGenerator;
 import org.junit.jupiter.api.Test;
 import org.opensearch.common.time.FormatNames;
+import org.opensearch.sql.data.type.ExprType;
 
 @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
 class OpenSearchDateTypeTest {
@@ -192,4 +194,11 @@ public void checkTimeFormatNames() {
         }
     );
   }
+
+  @Test
+  public void checkIfDateTypeCompatible() {
+    assertTrue(isDateTypeCompatible(DATE));
+    assertFalse(isDateTypeCompatible(OpenSearchDataType.of(
+        OpenSearchDataType.MappingType.Text)));
+  }
 }