Further rework on OpenSearchDataType:

* Add data types for those classes are not defined in `ExprCoreType`. * Add fixes to make all IT pass. * Complete some TODOs. Address #180 (comment) Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Bit-Quill · Dec 20, 2022 · 16d7ce0 · 16d7ce0
1 parent e4d89de
commit 16d7ce0
Show file tree

Hide file tree

Showing 24 changed files with 285 additions and 131 deletions.
diff --git a/core/src/main/java/org/opensearch/sql/expression/system/SystemFunctions.java b/core/src/main/java/org/opensearch/sql/expression/system/SystemFunctions.java
@@ -41,7 +41,7 @@ public Pair<FunctionSignature, FunctionBuilder> resolve(
             arguments -> new FunctionExpression(BuiltinFunctionName.TYPEOF.getName(), arguments) {
               @Override
               public ExprValue valueOf(Environment<Expression, ExprValue> valueEnv) {
-                return new ExprStringValue(getArguments().get(0).type().toString());
+                return new ExprStringValue(getArguments().get(0).type().typeName());
               }
 
               @Override

diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/PPLPluginIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/PPLPluginIT.java
@@ -45,7 +45,7 @@ public void testQueryEndpointShouldOK() throws IOException {
     client().performRequest(request);
 
     JSONObject response = executeQuery("search source=a");
-    verifySchema(response, schema("name", null, "string"));
+    verifySchema(response, schema("name", null, "text"));
     verifyDataRows(response, rows("hello"));
   }
 

diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/StandaloneIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/StandaloneIT.java
@@ -80,7 +80,7 @@ public void testSourceFieldQuery() throws IOException {
             + "  \"schema\": [\n"
             + "    {\n"
             + "      \"name\": \"name\",\n"
-            + "      \"type\": \"string\"\n"
+            + "      \"type\": \"text\"\n"
             + "    }\n"
             + "  ],\n"
             + "  \"datarows\": [\n"

diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/StatsCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/StatsCommandIT.java
@@ -119,7 +119,7 @@ public void testStatsWhere() throws IOException {
             "source=%s | stats sum(balance) as a by state | where a > 780000",
             TEST_INDEX_ACCOUNT));
     verifySchema(response, schema("a", null, "long"),
-        schema("state", null, "string"));
+        schema("state", null, "text"));
     verifyDataRows(response, rows(782199, "TX"));
   }
 

diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/SystemFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/SystemFunctionIT.java
@@ -68,7 +68,7 @@ public void typeof_opensearch_types() throws IOException {
         + " | fields `text`, `date`, `boolean`, `object`, `keyword`, `ip`, `binary`, `geo_point`",
             TEST_INDEX_DATATYPE_NONNUMERIC));
     verifyDataRows(response,
-        rows("OPENSEARCH_TEXT", "TIMESTAMP", "BOOLEAN", "STRUCT", "STRING",
-                "OPENSEARCH_IP", "OPENSEARCH_BINARY", "OPENSEARCH_GEO_POINT"));
+        rows("text", "TIMESTAMP", "BOOLEAN", "STRUCT", "STRING",
+                "ip", "binary", "geo_point"));
   }
 }
diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/SystemFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/SystemFunctionIT.java
@@ -54,7 +54,7 @@ public void typeof_opensearch_types() {
         //+ ", typeof(nested_value)"
         + " from %s;", TEST_INDEX_DATATYPE_NONNUMERIC));
     verifyDataRows(response,
-        rows("OPENSEARCH_TEXT", "TIMESTAMP", "BOOLEAN", "STRUCT", "STRING",
-                "OPENSEARCH_IP", "OPENSEARCH_BINARY", "OPENSEARCH_GEO_POINT"));
+        rows("text", "TIMESTAMP", "BOOLEAN", "STRUCT", "STRING",
+                "ip", "binary", "geo_point"));
   }
 }
diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchBinaryType.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchBinaryType.java
@@ -0,0 +1,20 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+
+package org.opensearch.sql.opensearch.data.type;
+
+import static org.opensearch.sql.data.type.ExprCoreType.UNKNOWN;
+
+import lombok.EqualsAndHashCode;
+
+@EqualsAndHashCode(callSuper = false)
+public class OpenSearchBinaryType extends OpenSearchDataType {
+
+  public OpenSearchBinaryType() {
+    super(Type.Binary);
+    exprCoreType = UNKNOWN;
+  }
+}
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
@@ -8,20 +8,25 @@
 
 import lombok.EqualsAndHashCode;
 import lombok.Getter;
-import org.opensearch.sql.data.type.ExprCoreType;
-import org.opensearch.sql.data.type.ExprType;
-
+import java.io.Serializable;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.stream.Collectors;
+import org.opensearch.sql.data.type.ExprCoreType;
+import org.opensearch.sql.data.type.ExprType;
 
 /**
  * The extension of ExprType in OpenSearch.
  */
 @EqualsAndHashCode
-public class OpenSearchDataType implements ExprType {
+public class OpenSearchDataType implements ExprType, Comparable<OpenSearchDataType>, Serializable {
+
+  @Override
+  public int compareTo(OpenSearchDataType o) {
+    return equals(o) ? 0 : 1;
+  }
 
   public enum Type {
     Text("text"),
@@ -30,6 +35,7 @@ public enum Type {
     GeoPoint("geo_point"),
     Binary("binary"),
     Date("date"),
+    Object("object"),
     Nested("nested"),
     Byte("byte"),
     Short("short"),
@@ -40,28 +46,38 @@ public enum Type {
     ScaledFloat("scaled_float"),
     Double("double"),
     Boolean("boolean");
-    // TODO: nested, object, ranges, geo shape, point, shape, scaled_float
+    // TODO: ranges, geo shape, point, shape
 
     private String name;
 
     Type(String name) {
       this.name = name;
     }
+
+    public String toString() {
+      return name;
+    }
   }
 
-  //@Getter
   @EqualsAndHashCode.Exclude
   private Type type;
 
   // resolved ExprCoreType
-  @Getter
-  private ExprCoreType exprCoreType;
+  protected ExprCoreType exprCoreType;
 
-  public OpenSearchDataType(Type type) {
-    this.type = type;
+  // Need to avoid returning `UNKNOWN` for `OpenSearch*Type`s, e.g. for IP
+  public ExprType getExprType() {
+    if (exprCoreType != ExprCoreType.UNKNOWN) {
+      return exprCoreType;
+    }
+    return this;
+  }
+
+  public static OpenSearchDataType of(Type type) {
+    ExprCoreType exprCoreType = ExprCoreType.UNKNOWN;
     switch (type) {
       // TODO update these 2 below #1038 https://github.com/opensearch-project/sql/issues/1038
-      case Text:
+      case Text: return new OpenSearchTextType();
       case Keyword: exprCoreType = ExprCoreType.STRING; break;
       case Byte: exprCoreType = ExprCoreType.BYTE; break;
       case Short: exprCoreType = ExprCoreType.SHORT; break;
@@ -74,35 +90,48 @@ public OpenSearchDataType(Type type) {
       case Boolean: exprCoreType = ExprCoreType.BOOLEAN; break;
       // TODO: check formats, it could allow TIME or DATE only
       case Date: exprCoreType = ExprCoreType.TIMESTAMP; break;
-      // TODO validate that it works ok, prev impl had `nested` -> ARRAY, `object` -> STRUCT
-      case Nested: exprCoreType = ExprCoreType.STRUCT; break;
-      // TODO
-      case GeoPoint:
-      case Binary:
-      case Ip:
+      case Object: exprCoreType = ExprCoreType.STRUCT; break;
+      case Nested: exprCoreType = ExprCoreType.ARRAY; break;
+      case GeoPoint: return new OpenSearchGeoPointType();
+      case Binary: return new OpenSearchBinaryType();
+      case Ip: return new OpenSearchIpType();
       default:
-        exprCoreType = ExprCoreType.UNKNOWN;
     }
+    var res = new OpenSearchDataType(type);
+    res.exprCoreType = exprCoreType;
+    return res;
   }
 
-  public OpenSearchDataType(ExprCoreType type) {
+  protected OpenSearchDataType(Type type) {
+    this.type = type;
+  }
+
+  public static OpenSearchDataType of(ExprType type) {
+    if (type instanceof OpenSearchDataType) {
+      return (OpenSearchDataType) type;
+    }
+    return new OpenSearchDataType((ExprCoreType) type);
+  }
+
+  protected OpenSearchDataType(ExprCoreType type) {
     this.exprCoreType = type;
-    // TODO set type?
   }
 
-  // nested has properties, text could have fields
-  // TODO could be fields in other types?
-  // TODO what is better structure - map (nested maps) or a tree?
+  protected OpenSearchDataType() { }
+
+  // object has properties
   @Getter
   @EqualsAndHashCode.Exclude
-  Map<String, OpenSearchDataType> fieldsOrProperties = new HashMap<>();
+  Map<String, OpenSearchDataType> properties = new HashMap<>();
+
+  // text could have fields
+  @Getter
+  @EqualsAndHashCode.Exclude
+  Map<String, OpenSearchDataType> fields = new HashMap<>();
 
-  public Boolean hasNestedFields() {
-    return fieldsOrProperties.size() > 0;
-  }
 
   /**
-   * Date formats stored in index mapping. Applicable for {@link Type.Date} only.
+   * Date formats stored in index mapping. Applicable for {@link Type#Date} only.
    */
   @Getter
   @EqualsAndHashCode.Exclude
@@ -119,30 +148,21 @@ public void setFormats(String formats) {
 
   @Override
   public String typeName() {
-    return type.toString();
-    /*
-    switch (type) {
-      case GeoPoint: return "geo_point";
-      case HalfFloat: return "half_point";
-      case ScaledFloat: return "scaled_point";
-      // TODO update these 2 below #1038 https://github.com/opensearch-project/sql/issues/1038
-      case Text: return "string";
-      case Keyword: return "string";
-      case Ip: return "ip";
-      case Binary: return "binary";
-      case Nested: return "nested";
-    }
-    throw new IllegalArgumentException(type.toString());
-    */
+    return type.toString().toLowerCase();
   }
 
   @Override
   public String legacyTypeName() {
-    throw new UnsupportedOperationException();
+    return typeName();
   }
 
-  private OpenSearchDataType cloneWithoutNested() {
-    var copy = new OpenSearchDataType(type);
+  /**
+   * Clone type object without {@link #properties} - without info nested about nested object types.
+   * @return A clone.
+   */
+  private OpenSearchDataType cloneEmpty() {
+    var copy = type != null ? of(type) : new OpenSearchDataType(exprCoreType);
+    copy.fields = fields; //TODO do we need to clone object?
     copy.exprCoreType = exprCoreType;
     copy.formats = formats;
     return copy;
@@ -152,15 +172,13 @@ public static Map<String, OpenSearchDataType> traverseAndFlatten(
       Map<String, OpenSearchDataType> tree) {
     Map<String, OpenSearchDataType> result = new HashMap<>();
     for (var entry : tree.entrySet()) {
-      result.put(entry.getKey(), entry.getValue().cloneWithoutNested());
-      if (entry.getValue().hasNestedFields()) { // can be omitted
-        result.putAll(
-            traverseAndFlatten(entry.getValue().fieldsOrProperties)
-                .entrySet().stream()
-                .collect(Collectors.toMap(
-                    e -> String.format("%s.%s", entry.getKey(), e.getKey()),
-                    e -> e.getValue())));
-      }
+      result.put(entry.getKey(), entry.getValue().cloneEmpty());
+      result.putAll(
+          traverseAndFlatten(entry.getValue().properties)
+              .entrySet().stream()
+              .collect(Collectors.toMap(
+                  e -> String.format("%s.%s", entry.getKey(), e.getKey()),
+                  e -> e.getValue())));
     }
     return result;
   }

diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchGeoPointType.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchGeoPointType.java
@@ -0,0 +1,20 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+
+package org.opensearch.sql.opensearch.data.type;
+
+import static org.opensearch.sql.data.type.ExprCoreType.UNKNOWN;
+
+import lombok.EqualsAndHashCode;
+
+@EqualsAndHashCode(callSuper = false)
+public class OpenSearchGeoPointType extends OpenSearchDataType {
+
+  public OpenSearchGeoPointType() {
+    super(Type.GeoPoint);
+    exprCoreType = UNKNOWN;
+  }
+}
diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchIpType.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchIpType.java
@@ -0,0 +1,20 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+
+package org.opensearch.sql.opensearch.data.type;
+
+import static org.opensearch.sql.data.type.ExprCoreType.UNKNOWN;
+
+import lombok.EqualsAndHashCode;
+
+@EqualsAndHashCode(callSuper = false)
+public class OpenSearchIpType extends OpenSearchDataType {
+
+  public OpenSearchIpType() {
+    super(Type.Ip);
+    exprCoreType = UNKNOWN;
+  }
+}
diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchTextType.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchTextType.java
@@ -0,0 +1,32 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package org.opensearch.sql.opensearch.data.type;
+
+import static org.opensearch.sql.data.type.ExprCoreType.STRING;
+import static org.opensearch.sql.data.type.ExprCoreType.UNKNOWN;
+
+import java.util.List;
+import org.opensearch.sql.data.type.ExprType;
+import lombok.EqualsAndHashCode;
+
+@EqualsAndHashCode(callSuper = false)
+public class OpenSearchTextType extends OpenSearchDataType {
+
+  public OpenSearchTextType() {
+    super(Type.Text);
+    exprCoreType = UNKNOWN;
+  }
+
+  @Override
+  public List<ExprType> getParent() {
+    return List.of(STRING);
+  }
+
+  @Override
+  public boolean shouldCast(ExprType other) {
+    return false;
+  }
+}
diff --git a/...rch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprBinaryValue.java b/...rch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprBinaryValue.java
@@ -12,6 +12,7 @@
 import org.opensearch.sql.data.model.AbstractExprValue;
 import org.opensearch.sql.data.model.ExprValue;
 import org.opensearch.sql.data.type.ExprType;
+import org.opensearch.sql.opensearch.data.type.OpenSearchBinaryType;
 import org.opensearch.sql.opensearch.data.type.OpenSearchDataType;
 
 
@@ -44,6 +45,6 @@ public Object value() {
 
   @Override
   public ExprType type() {
-    return new OpenSearchDataType(Binary);
+    return new OpenSearchBinaryType();
   }
 }
diff --git a/...h/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprGeoPointValue.java b/...h/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprGeoPointValue.java
@@ -14,6 +14,7 @@
 import org.opensearch.sql.data.model.ExprValue;
 import org.opensearch.sql.data.type.ExprType;
 import org.opensearch.sql.opensearch.data.type.OpenSearchDataType;
+import org.opensearch.sql.opensearch.data.type.OpenSearchGeoPointType;
 
 /**
  * OpenSearch GeoPointValue.
@@ -34,7 +35,7 @@ public Object value() {
 
   @Override
   public ExprType type() {
-    return new OpenSearchDataType(GeoPoint);
+    return new OpenSearchGeoPointType();
   }
 
   @Override