Assert that comparison only valid if same type, update tests accordingly

Signed-off-by: currantw <[email protected]>
Bit-Quill · Dec 3, 2024 · 5a8970e · 5a8970e
1 parent bca7f5f
commit 5a8970e
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 22 deletions.
diff --git a/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java b/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java
@@ -30,7 +30,17 @@ public class ExprIpValue extends AbstractExprValue {
           .toParams();
 
   public ExprIpValue(String s) {
-    value = stringToIpAddress(s);
+    try {
+      IPAddress address = new IPAddressString(s, validationOptions).toAddress();
+
+      // Convert IPv6 mapped IPv4 addresses to IPv4
+      if (address.isIPv4Convertible()) address = address.toIPv4();
+
+      value = address;
+    } catch (AddressStringException e) {
+      final String errorFormatString = "IP address string '%s' is not valid. Error details: %s";
+      throw new SemanticCheckException(String.format(errorFormatString, s, e.getMessage()));
+    }
   }
 
   @Override
@@ -45,14 +55,11 @@ public ExprType type() {
 
   @Override
   public int compare(ExprValue other) {
-    IPAddress otherValue =
-        other instanceof ExprIpValue exprIpValue
-            ? exprIpValue.value
-            : stringToIpAddress(other.stringValue());
+    assert (other instanceof ExprIpValue);
 
     // Map IPv4 addresses to IPv6 for comparison
     IPv6Address ipv6Value = toIPv6Address(value);
-    IPv6Address otherIpv6Value = toIPv6Address(otherValue);
+    IPv6Address otherIpv6Value = toIPv6Address(((ExprIpValue) other).value);
 
     return ipv6Value.compareTo(otherIpv6Value);
   }
@@ -67,17 +74,6 @@ public String toString() {
     return String.format("IP %s", value());
   }
 
-  /** Returns the {@link IPAddress} corresponding to the given {@link String}. */
-  private static IPAddress stringToIpAddress(String s) {
-    try {
-      IPAddress address = new IPAddressString(s, validationOptions).toAddress();
-      return address.isIPv4Convertible() ? address.toIPv4() : address;
-    } catch (AddressStringException e) {
-      final String errorFormatString = "IP address '%s' is not valid. Error details: %s";
-      throw new SemanticCheckException(String.format(errorFormatString, s, e.getMessage()));
-    }
-  }
-
   /** Returns the {@link IPv6Address} corresponding to the given {@link IPAddress}. */
   private static IPv6Address toIPv6Address(IPAddress ipAddress) {
     return ipAddress instanceof IPv4Address iPv4Address

diff --git a/core/src/test/java/org/opensearch/sql/data/model/ExprIpValueTest.java b/core/src/test/java/org/opensearch/sql/data/model/ExprIpValueTest.java
@@ -9,7 +9,6 @@
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
-import inet.ipaddr.AddressStringException;
 import java.util.List;
 import org.junit.jupiter.api.Test;
 import org.opensearch.sql.data.type.ExprCoreType;
@@ -57,11 +56,12 @@ public void testInvalid() {
     assertTrue(
         ex.getMessage()
             .matches(
-                String.format("IP address '%s' is not valid. Error details: .*", ipInvalidString)));
+                String.format(
+                    "IP address string '%s' is not valid. Error details: .*", ipInvalidString)));
   }
 
   @Test
-  public void testValue() throws AddressStringException {
+  public void testValue() {
     ipv4EqualStrings.forEach((s) -> assertEquals(ipv4String, ExprValueUtils.ipValue(s).value()));
     ipv6EqualStrings.forEach((s) -> assertEquals(ipv6String, ExprValueUtils.ipValue(s).value()));
   }

diff --git a/core/src/test/java/org/opensearch/sql/expression/operator/convert/TypeCastOperatorTest.java b/core/src/test/java/org/opensearch/sql/expression/operator/convert/TypeCastOperatorTest.java
@@ -364,7 +364,8 @@ void castToIp() {
 
     exp = DSL.castIp(DSL.literal(ipInvalidString));
     actualMsg = assertThrows(SemanticCheckException.class, exp::valueOf).getMessage();
-    expectedMsg = String.format("IP address '%s' is not valid. Error details: .*", ipInvalidString);
+    expectedMsg =
+        String.format("IP address string '%s' is not valid. Error details: .*", ipInvalidString);
     assertTrue(actualMsg.matches(expectedMsg));
 
     // From IP address

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
@@ -160,6 +160,8 @@ public static OpenSearchDataType of(MappingType mappingType, Map<String, Object>
         return OpenSearchGeoPointType.of();
       case Binary:
         return OpenSearchBinaryType.of();
+      case Ip:
+        return OpenSearchIpType.of();
       case Date:
       case DateNanos:
         // Default date formatter is used when "" is passed as the second parameter

diff --git a/...rc/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java b/...rc/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java
@@ -59,6 +59,7 @@
 import org.opensearch.sql.data.model.ExprValue;
 import org.opensearch.sql.opensearch.data.type.OpenSearchDataType;
 import org.opensearch.sql.opensearch.data.type.OpenSearchDateType;
+import org.opensearch.sql.opensearch.data.type.OpenSearchIpType;
 import org.opensearch.sql.opensearch.data.type.OpenSearchTextType;
 import org.opensearch.sql.opensearch.data.utils.OpenSearchJsonContent;
 
@@ -116,7 +117,7 @@ class OpenSearchExprValueFactoryTest {
               "textKeywordV",
               OpenSearchTextType.of(
                   Map.of("words", OpenSearchDataType.of(OpenSearchDataType.MappingType.Keyword))))
-          .put(fieldIp, OpenSearchDataType.of(OpenSearchDataType.MappingType.Ip))
+          .put(fieldIp, OpenSearchIpType.of(OpenSearchDataType.MappingType.Ip))
           .put("geoV", OpenSearchDataType.of(OpenSearchDataType.MappingType.GeoPoint))
           .put("binaryV", OpenSearchDataType.of(OpenSearchDataType.MappingType.Binary))
           .build();