Revert changes to cast IP to string.

Signed-off-by: currantw <[email protected]>

core/src/test/java/org/opensearch/sql/expression/aggregation/PercentileApproxAggregatorTest.java

@@ -13,12 +13,18 @@
 
 package org.opensearch.sql.expression.aggregation;
 
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.when;
 import static org.opensearch.sql.data.model.ExprValueUtils.integerValue;
 import static org.opensearch.sql.data.model.ExprValueUtils.longValue;
-import static org.opensearch.sql.data.type.ExprCoreType.*;
+import static org.opensearch.sql.data.type.ExprCoreType.DOUBLE;
+import static org.opensearch.sql.data.type.ExprCoreType.FLOAT;
+import static org.opensearch.sql.data.type.ExprCoreType.INTEGER;
+import static org.opensearch.sql.data.type.ExprCoreType.LONG;
+import static org.opensearch.sql.data.type.ExprCoreType.STRING;
 
 import java.util.ArrayList;
 import java.util.List;

integ-test/src/test/java/org/opensearch/sql/legacy/JdbcTestIT.java

@@ -157,7 +157,7 @@ public void ipTypeShouldPassJdbcFormatter() {
         executeQuery(
             "SELECT host AS hostIP FROM " + TestsConstants.TEST_INDEX_WEBLOG + " ORDER BY hostIP",
             "jdbc"),
-        containsString("\"type\": \"string\""));
+        containsString("\"type\": \"ip\""));
   }
 
   @Test

integ-test/src/test/java/org/opensearch/sql/ppl/DataTypeIT.java

@@ -50,7 +50,7 @@ public void test_nonnumeric_data_types() throws IOException {
         schema("binary_value", "binary"),
         schema("date_value", "timestamp"),
         schema("date_nanos_value", "timestamp"),
-        schema("ip_value", "string"),
+        schema("ip_value", "ip"),
         schema("object_value", "struct"),
         schema("nested_value", "array"),
         schema("geo_point_value", "geo_point"));

integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java

@@ -31,27 +31,27 @@ public void test_cidrmatch() throws IOException {
     result =
         executeQuery(
             String.format(
-                "source=%s | where cidrmatch(host, '199.120.111.0/24') | fields host",
+                "source=%s | where cidrmatch(host_string, '199.120.111.0/24') | fields host_string",
                 TEST_INDEX_WEBLOG));
-    verifySchema(result, schema("host", null, "string"));
+    verifySchema(result, schema("host_string", null, "string"));
     verifyDataRows(result);
 
     // One match
     result =
         executeQuery(
             String.format(
-                "source=%s | where cidrmatch(host, '199.120.110.0/24') | fields host",
+                "source=%s | where cidrmatch(host_string, '199.120.110.0/24') | fields host_string",
                 TEST_INDEX_WEBLOG));
-    verifySchema(result, schema("host", null, "string"));
+    verifySchema(result, schema("host_string", null, "string"));
     verifyDataRows(result, rows("199.120.110.21"));
 
     // Multiple matches
     result =
         executeQuery(
             String.format(
-                "source=%s | where cidrmatch(host, '199.0.0.0/8') | fields host",
+                "source=%s | where cidrmatch(host_string, '199.0.0.0/8') | fields host_string",
                 TEST_INDEX_WEBLOG));
-    verifySchema(result, schema("host", null, "string"));
+    verifySchema(result, schema("host_string", null, "string"));
     verifyDataRows(result, rows("199.72.81.55"), rows("199.120.110.21"));
   }
 }

integ-test/src/test/java/org/opensearch/sql/ppl/SystemFunctionIT.java

@@ -89,7 +89,7 @@ public void typeof_opensearch_types() throws IOException {
             "BOOLEAN",
             "OBJECT",
             "KEYWORD",
-            "KEYWORD",
+            "IP",
             "BINARY",
             "GEO_POINT"));
   }

integ-test/src/test/resources/indexDefinitions/weblogs_index_mapping.json

@@ -1,9 +1,12 @@
 {
   "mappings": {
     "properties": {
-      "host": {
+      "host_ip": {
         "type": "ip"
       },
+      "host_string": {
+        "type": "keyword"
+      },
       "method": {
         "type": "text"
       },

integ-test/src/test/resources/weblogs.json

@@ -1,6 +1,6 @@
 {"index":{}}
-{"host": "199.72.81.55", "method": "GET", "url": "/history/apollo/", "response": "200", "bytes": "6245"}
+{"host_ip": "199.72.81.55", "host_string": "199.72.81.55", "method": "GET", "url": "/history/apollo/", "response": "200", "bytes": "6245"}
 {"index":{}}
-{"host": "199.120.110.21", "method": "GET", "url": "/shuttle/missions/sts-73/mission-sts-73.html", "response": "200", "bytes": "4085"}
+{"host_ip": "199.120.110.21", "host_string": "199.120.110.21", "method": "GET", "url": "/shuttle/missions/sts-73/mission-sts-73.html", "response": "200", "bytes": "4085"}
 {"index":{}}
-{"host": "205.212.115.106", "method": "GET", "url": "/shuttle/countdown/countdown.html", "response": "200", "bytes": "3985"}
+{"host_ip": "205.212.115.106", "host_string": "205.212.115.106", "method": "GET", "url": "/shuttle/countdown/countdown.html", "response": "200", "bytes": "3985"}

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

@@ -26,7 +26,7 @@ public enum MappingType {
     Invalid(null, ExprCoreType.UNKNOWN),
     Text("text", ExprCoreType.UNKNOWN),
     Keyword("keyword", ExprCoreType.STRING),
-    Ip("ip", ExprCoreType.STRING),
+    Ip("ip", ExprCoreType.UNKNOWN),
     GeoPoint("geo_point", ExprCoreType.UNKNOWN),
     Binary("binary", ExprCoreType.UNKNOWN),
     Date("date", ExprCoreType.TIMESTAMP),

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

@@ -5,7 +5,7 @@
 
 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 lombok.EqualsAndHashCode;
 
@@ -20,7 +20,7 @@ public class OpenSearchIpType extends OpenSearchDataType {
 
   private OpenSearchIpType() {
     super(MappingType.Ip);
-    exprCoreType = STRING;
+    exprCoreType = UNKNOWN;
   }
 
   public static OpenSearchIpType of() {

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

@@ -45,9 +45,4 @@ public boolean equal(ExprValue other) {
   public int hashCode() {
     return Objects.hashCode(ip);
   }
-
-  @Override
-  public String stringValue() {
-    return ip;
-  }
 }

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

@@ -71,8 +71,32 @@
 
 /** Construct ExprValue from OpenSearch response. */
 public class OpenSearchExprValueFactory {
+  /** The Mapping of Field and ExprType. */
+  private final Map<String, OpenSearchDataType> typeMapping;
+
+  /** Whether to support nested value types (such as arrays) */
+  private final boolean fieldTypeTolerance;
+
+  /**
+   * Extend existing mapping by new data without overwrite. Called from aggregation only {@see
+   * AggregationQueryBuilder#buildTypeMapping}.
+   *
+   * @param typeMapping A data type mapping produced by aggregation.
+   */
+  public void extendTypeMapping(Map<String, OpenSearchDataType> typeMapping) {
+    for (var field : typeMapping.keySet()) {
+      // Prevent overwriting, because aggregation engine may be not aware
+      // of all niceties of all types.
+      this.typeMapping.putIfAbsent(field, typeMapping.get(field));
+    }
+  }
+
+  @Getter @Setter private OpenSearchAggregationResponseParser parser;
+
   private static final String TOP_PATH = "";
+
   private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+
   private static final Map<ExprType, BiFunction<Content, ExprType, ExprValue>> typeActionMap =
       new ImmutableMap.Builder<ExprType, BiFunction<Content, ExprType, ExprValue>>()
           .put(
@@ -110,27 +134,93 @@ public class OpenSearchExprValueFactory {
               OpenSearchExprValueFactory::createOpenSearchDateType)
           .put(
               OpenSearchDataType.of(OpenSearchDataType.MappingType.Ip),
-              (c, dt) -> new ExprStringValue(c.stringValue()))
+              (c, dt) -> new OpenSearchExprIpValue(c.stringValue()))
           .put(
               OpenSearchDataType.of(OpenSearchDataType.MappingType.Binary),
               (c, dt) -> new OpenSearchExprBinaryValue(c.stringValue()))
           .build();
 
-  /** The Mapping of Field and ExprType. */
-  private final Map<String, OpenSearchDataType> typeMapping;
-
-  /** Whether to support nested value types (such as arrays) */
-  private final boolean fieldTypeTolerance;
-
-  @Getter @Setter private OpenSearchAggregationResponseParser parser;
-
   /** Constructor of OpenSearchExprValueFactory. */
   public OpenSearchExprValueFactory(
       Map<String, OpenSearchDataType> typeMapping, boolean fieldTypeTolerance) {
     this.typeMapping = OpenSearchDataType.traverseAndFlatten(typeMapping);
     this.fieldTypeTolerance = fieldTypeTolerance;
   }
 
+  /**
+   *
+   *
+   * <pre>
+   * The struct construction has the following assumption:
+   *  1. The field has OpenSearch Object data type.
+   *     See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/object.html">
+   *       docs</a>
+   *  2. The deeper field is flattened in the typeMapping. e.g.
+   *     { "employ",       "STRUCT"  }
+   *     { "employ.id",    "INTEGER" }
+   *     { "employ.state", "STRING"  }
+   *  </pre>
+   */
+  public ExprValue construct(String jsonString, boolean supportArrays) {
+    try {
+      return parse(
+          new OpenSearchJsonContent(OBJECT_MAPPER.readTree(jsonString)),
+          TOP_PATH,
+          Optional.of(STRUCT),
+          fieldTypeTolerance || supportArrays);
+    } catch (JsonProcessingException e) {
+      throw new IllegalStateException(String.format("invalid json: %s.", jsonString), e);
+    }
+  }
+
+  /**
+   * Construct ExprValue from field and its value object. Throw exception if trying to construct
+   * from field of unsupported type.<br>
+   * Todo, add IP, GeoPoint support after we have function implementation around it.
+   *
+   * @param field field name
+   * @param value value object
+   * @return ExprValue
+   */
+  public ExprValue construct(String field, Object value, boolean supportArrays) {
+    return parse(new ObjectContent(value), field, type(field), supportArrays);
+  }
+
+  private ExprValue parse(
+      Content content, String field, Optional<ExprType> fieldType, boolean supportArrays) {
+    if (content.isNull() || !fieldType.isPresent()) {
+      return ExprNullValue.of();
+    }
+
+    final ExprType type = fieldType.get();
+
+    if (type.equals(OpenSearchDataType.of(OpenSearchDataType.MappingType.GeoPoint))) {
+      return parseGeoPoint(content, supportArrays);
+    } else if (type.equals(OpenSearchDataType.of(OpenSearchDataType.MappingType.Nested))
+        || content.isArray()) {
+      return parseArray(content, field, type, supportArrays);
+    } else if (type.equals(OpenSearchDataType.of(OpenSearchDataType.MappingType.Object))
+        || type == STRUCT) {
+      return parseStruct(content, field, supportArrays);
+    } else {
+      if (typeActionMap.containsKey(type)) {
+        return typeActionMap.get(type).apply(content, type);
+      } else {
+        throw new IllegalStateException(
+            String.format(
+                "Unsupported type: %s for value: %s.", type.typeName(), content.objectValue()));
+      }
+    }
+  }
+
+  /**
+   * In OpenSearch, it is possible field doesn't have type definition in mapping. but has empty
+   * value. For example, {"empty_field": []}.
+   */
+  private Optional<ExprType> type(String field) {
+    return Optional.ofNullable(typeMapping.get(field));
+  }
+
   /**
    * Parse value with the first matching formatter into {@link ExprValue} with corresponding {@link
    * ExprCoreType}.
@@ -218,92 +308,6 @@ private static ExprValue createOpenSearchDateType(Content value, ExprType type)
     return new ExprTimestampValue((Instant) value.objectValue());
   }
 
-  /**
-   * Extend existing mapping by new data without overwrite. Called from aggregation only {@see
-   * AggregationQueryBuilder#buildTypeMapping}.
-   *
-   * @param typeMapping A data type mapping produced by aggregation.
-   */
-  public void extendTypeMapping(Map<String, OpenSearchDataType> typeMapping) {
-    for (var field : typeMapping.keySet()) {
-      // Prevent overwriting, because aggregation engine may be not aware
-      // of all niceties of all types.
-      this.typeMapping.putIfAbsent(field, typeMapping.get(field));
-    }
-  }
-
-  /**
-   *
-   *
-   * <pre>
-   * The struct construction has the following assumption:
-   *  1. The field has OpenSearch Object data type.
-   *     See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/object.html">
-   *       docs</a>
-   *  2. The deeper field is flattened in the typeMapping. e.g.
-   *     { "employ",       "STRUCT"  }
-   *     { "employ.id",    "INTEGER" }
-   *     { "employ.state", "STRING"  }
-   *  </pre>
-   */
-  public ExprValue construct(String jsonString, boolean supportArrays) {
-    try {
-      return parse(
-          new OpenSearchJsonContent(OBJECT_MAPPER.readTree(jsonString)),
-          TOP_PATH,
-          Optional.of(STRUCT),
-          fieldTypeTolerance || supportArrays);
-    } catch (JsonProcessingException e) {
-      throw new IllegalStateException(String.format("invalid json: %s.", jsonString), e);
-    }
-  }
-
-  /**
-   * Construct ExprValue from field and its value object. Throw exception if trying to construct
-   * from field of unsupported type.<br>
-   * Todo, add IP, GeoPoint support after we have function implementation around it.
-   *
-   * @param field field name
-   * @param value value object
-   * @return ExprValue
-   */
-  public ExprValue construct(String field, Object value, boolean supportArrays) {
-    return parse(new ObjectContent(value), field, type(field), supportArrays);
-  }
-
-  private ExprValue parse(
-      Content content, String field, Optional<ExprType> fieldType, boolean supportArrays) {
-    if (content.isNull() || !fieldType.isPresent()) {
-      return ExprNullValue.of();
-    }
-
-    final ExprType type = fieldType.get();
-
-    if (type.equals(OpenSearchDataType.of(OpenSearchDataType.MappingType.GeoPoint))) {
-      return parseGeoPoint(content, supportArrays);
-    } else if (type.equals(OpenSearchDataType.of(OpenSearchDataType.MappingType.Nested))
-        || content.isArray()) {
-      return parseArray(content, field, type, supportArrays);
-    } else if (type.equals(OpenSearchDataType.of(OpenSearchDataType.MappingType.Object))
-        || type == STRUCT) {
-      return parseStruct(content, field, supportArrays);
-    } else if (typeActionMap.containsKey(type)) {
-      return typeActionMap.get(type).apply(content, type);
-    } else {
-      throw new IllegalStateException(
-          String.format(
-              "Unsupported type: %s for value: %s.", type.typeName(), content.objectValue()));
-    }
-  }
-
-  /**
-   * In OpenSearch, it is possible field doesn't have type definition in mapping. but has empty
-   * value. For example, {"empty_field": []}.
-   */
-  private Optional<ExprType> type(String field) {
-    return Optional.ofNullable(typeMapping.get(field));
-  }
-
   /**
    * Parse struct content.
    *