Address PR feedback.

* Make string representations of all `ExprType`s uppercase.
* Remove functions from `IndexMapping` used in tests only.
* Change `OpenSearchDataType::cloneEmpty` function to avoid using reflection; update derived classes to override it.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java

@@ -81,9 +81,9 @@ public enum ExprCoreType implements ExprType {
    */
   private static final Map<ExprCoreType, String> LEGACY_TYPE_NAME_MAPPING =
       new ImmutableMap.Builder<ExprCoreType, String>()
-          .put(STRUCT, "object")
-          .put(ARRAY, "nested")
-          .put(STRING, "keyword")
+          .put(STRUCT, "OBJECT")
+          .put(ARRAY, "NESTED")
+          .put(STRING, "KEYWORD")
           .build();
 
   private static final Set<ExprType> NUMBER_TYPES =

core/src/test/java/org/opensearch/sql/expression/system/SystemFunctionsTest.java

@@ -47,7 +47,7 @@ public class SystemFunctionsTest {
   void typeof() {
     assertEquals(STRING, dsl.typeof(DSL.literal(1)).type());
 
-    assertEquals("nested", typeofGetValue(new ExprCollectionValue(List.of())));
+    assertEquals("NESTED", typeofGetValue(new ExprCollectionValue(List.of())));
     assertEquals("BOOLEAN", typeofGetValue(ExprBooleanValue.of(false)));
     assertEquals("BYTE", typeofGetValue(new ExprByteValue(0)));
     assertEquals("DATE", typeofGetValue(new ExprDateValue(LocalDate.now())));
@@ -58,8 +58,8 @@ void typeof() {
     assertEquals("INTERVAL", typeofGetValue(new ExprIntervalValue(Duration.ofDays(0))));
     assertEquals("LONG", typeofGetValue(new ExprLongValue(0)));
     assertEquals("SHORT", typeofGetValue(new ExprShortValue(0)));
-    assertEquals("STRING", typeofGetValue(new ExprStringValue("")));
-    assertEquals("STRUCT", typeofGetValue(new ExprTupleValue(new LinkedHashMap<>())));
+    assertEquals("KEYWORD", typeofGetValue(new ExprStringValue("")));
+    assertEquals("OBJECT", typeofGetValue(new ExprTupleValue(new LinkedHashMap<>())));
     assertEquals("TIME", typeofGetValue(new ExprTimeValue(LocalTime.now())));
     assertEquals("TIMESTAMP", typeofGetValue(new ExprTimestampValue(Instant.now())));
     assertEquals("UNDEFINED", typeofGetValue(ExprNullValue.of()));

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

@@ -25,4 +25,9 @@ private OpenSearchBinaryType() {
     super(MappingType.Binary);
     exprCoreType = UNKNOWN;
   }
+
+  @Override
+  protected OpenSearchDataType cloneEmpty() {
+    return instance;
+  }
 }

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

@@ -14,7 +14,6 @@
 import java.util.function.BiConsumer;
 import lombok.EqualsAndHashCode;
 import lombok.Getter;
-import lombok.SneakyThrows;
 import org.opensearch.sql.data.type.ExprCoreType;
 import org.opensearch.sql.data.type.ExprType;
 
@@ -28,30 +27,35 @@ public class OpenSearchDataType implements ExprType, Serializable {
    * The mapping (OpenSearch engine) type.
    */
   public enum MappingType {
-    Invalid(null),
-    Text("text"),
-    Keyword("keyword"),
-    Ip("ip"),
-    GeoPoint("geo_point"),
-    Binary("binary"),
-    Date("date"),
-    Object("object"),
-    Nested("nested"),
-    Byte("byte"),
-    Short("short"),
-    Integer("integer"),
-    Long("long"),
-    Float("float"),
-    HalfFloat("half_float"),
-    ScaledFloat("scaled_float"),
-    Double("double"),
-    Boolean("boolean");
+    Invalid(null, ExprCoreType.UNKNOWN),
+    Text("text", ExprCoreType.UNKNOWN),
+    Keyword("keyword", ExprCoreType.STRING),
+    Ip("ip", ExprCoreType.UNKNOWN),
+    GeoPoint("geo_point", ExprCoreType.UNKNOWN),
+    Binary("binary", ExprCoreType.UNKNOWN),
+    Date("date", ExprCoreType.TIMESTAMP),
+    Object("object", ExprCoreType.STRUCT),
+    Nested("nested", ExprCoreType.ARRAY),
+    Byte("byte", ExprCoreType.BYTE),
+    Short("short", ExprCoreType.SHORT),
+    Integer("integer", ExprCoreType.INTEGER),
+    Long("long", ExprCoreType.LONG),
+    Float("float", ExprCoreType.FLOAT),
+    HalfFloat("half_float", ExprCoreType.FLOAT),
+    ScaledFloat("scaled_float", ExprCoreType.DOUBLE),
+    Double("double", ExprCoreType.DOUBLE),
+    Boolean("boolean", ExprCoreType.BOOLEAN);
     // TODO: ranges, geo shape, point, shape
 
-    private String name;
+    private final String name;
 
-    MappingType(String name) {
+    // Associated `ExprCoreType`
+    @Getter
+    private final ExprCoreType exprCoreType;
+
+    MappingType(String name, ExprCoreType exprCoreType) {
       this.name = name;
+      this.exprCoreType = exprCoreType;
     }
 
     public String toString() {
@@ -93,42 +97,17 @@ public static OpenSearchDataType of(MappingType mappingType) {
     if (instances.containsKey(mappingType.toString())) {
       return instances.get(mappingType.toString());
     }
-    ExprCoreType exprCoreType = ExprCoreType.UNKNOWN;
-    switch (mappingType) {
-      // TODO update these 2 below #1038 https://github.com/opensearch-project/sql/issues/1038
-      case Text: return OpenSearchTextType.getInstance();
-      case Keyword: exprCoreType = ExprCoreType.STRING;
-        break;
-      case Byte: exprCoreType = ExprCoreType.BYTE;
-        break;
-      case Short: exprCoreType = ExprCoreType.SHORT;
-        break;
-      case Integer: exprCoreType = ExprCoreType.INTEGER;
-        break;
-      case Long: exprCoreType = ExprCoreType.LONG;
-        break;
-      case HalfFloat: exprCoreType = ExprCoreType.FLOAT;
-        break;
-      case Float: exprCoreType = ExprCoreType.FLOAT;
-        break;
-      case ScaledFloat: exprCoreType = ExprCoreType.DOUBLE;
-        break;
-      case Double: exprCoreType = ExprCoreType.DOUBLE;
-        break;
-      case Boolean: exprCoreType = ExprCoreType.BOOLEAN;
-        break;
-      // TODO: check formats, it could allow TIME or DATE only
-      case Date: exprCoreType = ExprCoreType.TIMESTAMP;
-        break;
-      case Object: exprCoreType = ExprCoreType.STRUCT;
-        break;
-      case Nested: exprCoreType = ExprCoreType.ARRAY;
-        break;
-      case GeoPoint: return OpenSearchGeoPointType.getInstance();
-      case Binary: return OpenSearchBinaryType.getInstance();
-      case Ip: return OpenSearchIpType.getInstance();
-      default:
-        throw new IllegalArgumentException(mappingType.toString());
+    ExprCoreType exprCoreType = mappingType.getExprCoreType();
+    if (exprCoreType == ExprCoreType.UNKNOWN) {
+      switch (mappingType) {
+        // TODO update these 2 below #1038 https://github.com/opensearch-project/sql/issues/1038
+        case Text: return OpenSearchTextType.getInstance();
+        case GeoPoint: return OpenSearchGeoPointType.getInstance();
+        case Binary: return OpenSearchBinaryType.getInstance();
+        case Ip: return OpenSearchIpType.getInstance();
+        default:
+          throw new IllegalArgumentException(mappingType.toString());
+      }
     }
     var res = new OpenSearchDataType(mappingType);
     res.exprCoreType = exprCoreType;
@@ -201,8 +180,8 @@ protected OpenSearchDataType() {
   public String typeName() {
     // To avoid breaking changes return `string` for `typeName` call (PPL) and `text` for
     // `legacyTypeName` call (SQL). See more: https://github.com/opensearch-project/sql/issues/1296
-    if (legacyTypeName().equals("text")) {
-      return "string";
+    if (legacyTypeName().equals("TEXT")) {
+      return "STRING";
     }
     return legacyTypeName();
   }
@@ -213,24 +192,17 @@ public String legacyTypeName() {
     if (mappingType == null) {
       return exprCoreType.typeName();
     }
-    return mappingType.toString().toLowerCase();
+    return mappingType.toString().toUpperCase();
   }
 
   /**
    * Clone type object without {@link #properties} - without info about nested object types.
+   * Note: Should be overriden by all derived classes for proper work.
    * @return A cloned object.
    */
-  @SneakyThrows
   protected OpenSearchDataType cloneEmpty() {
-    // This trick is required to ensure that the clone has the same type as clonee.
-    // Otherwise, clone of OpenSearchTextType becomes OpenSearchDataType.
-    // An alternate option is to @Override this function in all inheritors.
-    // Requires all derived types to have a default constructor.
-    var ctor = this.getClass().getDeclaredConstructor();
-    ctor.setAccessible(true);
-    var copy = (OpenSearchDataType)ctor.newInstance();
+    var copy = new OpenSearchDataType();
     copy.mappingType = mappingType;
-    copy.fields = fields;
     copy.exprCoreType = exprCoreType;
     return copy;
   }

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

@@ -25,4 +25,9 @@ private OpenSearchGeoPointType() {
     super(MappingType.GeoPoint);
     exprCoreType = UNKNOWN;
   }
+
+  @Override
+  protected OpenSearchDataType cloneEmpty() {
+    return instance;
+  }
 }

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

@@ -25,4 +25,9 @@ private OpenSearchIpType() {
     super(MappingType.Ip);
     exprCoreType = UNKNOWN;
   }
+
+  @Override
+  protected OpenSearchDataType cloneEmpty() {
+    return instance;
+  }
 }

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

@@ -49,6 +49,11 @@ public Map<String, OpenSearchDataType> getFields() {
     return fields;
   }
 
+  @Override
+  protected OpenSearchDataType cloneEmpty() {
+    return new OpenSearchTextType(fields);
+  }
+
   /**
    * Text field doesn't have doc value (exception thrown even when you call "get")
    * Limitation: assume inner field name is always "keyword".

opensearch/src/main/java/org/opensearch/sql/opensearch/mapping/IndexMapping.java

@@ -27,20 +27,6 @@ public class IndexMapping {
   @Getter
   private final Map<String, OpenSearchDataType> fieldMappings;
 
-  // TODO remove, used in tests only
-  /**
-   * A constructor for testing purposes only.
-   * @param fieldMappings Mapping info in format field name - field OpenSearch type.
-   */
-  public IndexMapping(Map<String, String> fieldMappings) {
-    this.fieldMappings = fieldMappings.entrySet().stream()
-        .filter(e -> EnumUtils.isValidEnumIgnoreCase(
-            OpenSearchDataType.MappingType.class, e.getValue()))
-        .collect(Collectors.toMap(e -> e.getKey(), e -> OpenSearchDataType.of(
-            EnumUtils.getEnumIgnoreCase(OpenSearchDataType.MappingType.class, e.getValue()))
-        ));
-  }
-
   @SuppressWarnings("unchecked")
   public IndexMapping(MappingMetadata metaData) {
     this.fieldMappings = parseMapping((Map<String, Object>) metaData.getSourceAsMap()
@@ -56,19 +42,6 @@ public int size() {
     return fieldMappings.size();
   }
 
-  /**
-   * Return field type by its name.
-   *
-   * @param fieldName field name
-   * @return field type in string. Or null if not exist.
-   */
-  public String getFieldType(String fieldName) {
-    if (!fieldMappings.containsKey(fieldName)) {
-      return null;
-    }
-    return fieldMappings.get(fieldName).legacyTypeName();
-  }
-
   @SuppressWarnings("unchecked")
   private Map<String, OpenSearchDataType> parseMapping(Map<String, Object> indexMapping) {
     Map<String, OpenSearchDataType> result = new LinkedHashMap<>();

opensearch/src/test/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClientTest.java

@@ -22,6 +22,7 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static org.opensearch.sql.opensearch.client.OpenSearchClient.META_CLUSTER_NAME;
+import static org.opensearch.sql.opensearch.data.type.OpenSearchDataType.MappingType;
 
 import com.google.common.base.Charsets;
 import com.google.common.collect.ImmutableMap;
@@ -101,64 +102,64 @@ public void getIndexMappings() throws IOException {
 
     Map<String, IndexMapping> indexMappings = client.getIndexMappings(indexName);
 
-    IndexMapping indexMapping = indexMappings.values().iterator().next();
-    var parsedTypes = OpenSearchDataType.traverseAndFlatten(indexMapping.getFieldMappings());
+    var mapping = indexMappings.values().iterator().next().getFieldMappings();
+    var parsedTypes = OpenSearchDataType.traverseAndFlatten(mapping);
     assertAll(
         () -> assertEquals(1, indexMappings.size()),
         // 10 types extended to 17 after flattening
-        () -> assertEquals(10, indexMapping.size()),
+        () -> assertEquals(10, mapping.size()),
         () -> assertEquals(17, parsedTypes.size()),
-        () -> assertEquals("text", indexMapping.getFieldType("address")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Text),
+        () -> assertEquals("TEXT", mapping.get("address").legacyTypeName()),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Text),
             parsedTypes.get("address")),
-        () -> assertEquals("integer", indexMapping.getFieldType("age")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Integer),
+        () -> assertEquals("INTEGER", mapping.get("age").legacyTypeName()),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Integer),
             parsedTypes.get("age")),
-        () -> assertEquals("double", indexMapping.getFieldType("balance")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Double),
+        () -> assertEquals("DOUBLE", mapping.get("balance").legacyTypeName()),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Double),
             parsedTypes.get("balance")),
-        () -> assertEquals("keyword", indexMapping.getFieldType("city")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Keyword),
+        () -> assertEquals("KEYWORD", mapping.get("city").legacyTypeName()),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Keyword),
             parsedTypes.get("city")),
-        () -> assertEquals("date", indexMapping.getFieldType("birthday")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Date),
+        () -> assertEquals("DATE", mapping.get("birthday").legacyTypeName()),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Date),
             parsedTypes.get("birthday")),
-        () -> assertEquals("geo_point", indexMapping.getFieldType("location")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.GeoPoint),
+        () -> assertEquals("GEO_POINT", mapping.get("location").legacyTypeName()),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.GeoPoint),
             parsedTypes.get("location")),
         // unknown type isn't parsed and ignored
-        () -> assertNull(indexMapping.getFieldType("new_field")),
+        () -> assertFalse(mapping.containsKey("new_field")),
         () -> assertNull(parsedTypes.get("new_field")),
-        () -> assertEquals("text", indexMapping.getFieldType("field with spaces")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Text),
+        () -> assertEquals("TEXT", mapping.get("field with spaces").legacyTypeName()),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Text),
             parsedTypes.get("field with spaces")),
-        () -> assertEquals("text", indexMapping.getFieldType("employer")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Text),
+        () -> assertEquals("TEXT", mapping.get("employer").legacyTypeName()),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Text),
             parsedTypes.get("employer")),
         // `employer` is a `text` with `fields`
         () -> assertTrue(((OpenSearchTextType)parsedTypes.get("employer")).getFields().size() > 0),
-        () -> assertEquals("nested", indexMapping.getFieldType("projects")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Nested),
+        () -> assertEquals("NESTED", mapping.get("projects").legacyTypeName()),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Nested),
             parsedTypes.get("projects")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Boolean),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Boolean),
             parsedTypes.get("projects.active")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Date),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Date),
             parsedTypes.get("projects.release")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Nested),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Nested),
             parsedTypes.get("projects.members")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Text),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Text),
             parsedTypes.get("projects.members.name")),
-        () -> assertEquals("object", indexMapping.getFieldType("manager")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Object),
+        () -> assertEquals("OBJECT", mapping.get("manager").legacyTypeName()),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Object),
                 parsedTypes.get("manager")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Text),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Text),
             parsedTypes.get("manager.name")),
         // `manager.name` is a `text` with `fields`
         () -> assertTrue(((OpenSearchTextType)parsedTypes.get("manager.name"))
                 .getFields().size() > 0),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Keyword),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Keyword),
             parsedTypes.get("manager.address")),
-        () -> assertEquals(OpenSearchTextType.of(OpenSearchDataType.MappingType.Long),
+        () -> assertEquals(OpenSearchTextType.of(MappingType.Long),
             parsedTypes.get("manager.salary"))
     );
   }