From a8da96d4ecc9a4fa8be157d310f3187b82efd5bb Mon Sep 17 00:00:00 2001 From: currantw Date: Wed, 27 Nov 2024 09:00:25 -0800 Subject: [PATCH 01/26] Add `ExprIpValue` and `IP` data type Signed-off-by: currantw --- .../sql/data/model/ExprIpValue.java | 64 +++++++++++ .../sql/data/model/ExprValueUtils.java | 4 + .../sql/data/type/ExprCoreType.java | 3 + .../opensearch/sql/analysis/AnalyzerTest.java | 2 +- .../sql/data/model/ExprIpValueTest.java | 106 ++++++++++++++++++ .../function/WideningTypeRuleTest.java | 2 + .../data/type/OpenSearchDataType.java | 2 +- .../data/type/OpenSearchIpType.java | 4 +- .../data/value/OpenSearchExprIpValue.java | 48 -------- .../value/OpenSearchExprValueFactory.java | 5 +- .../OpenSearchDataTypeRecognitionTest.java | 2 - .../data/type/OpenSearchDataTypeTest.java | 7 +- .../data/value/OpenSearchExprIpValueTest.java | 44 -------- .../value/OpenSearchExprValueFactoryTest.java | 44 +++++--- 14 files changed, 219 insertions(+), 118 deletions(-) create mode 100644 core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java create mode 100644 core/src/test/java/org/opensearch/sql/data/model/ExprIpValueTest.java delete mode 100644 opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprIpValue.java 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 new file mode 100644 index 00000000000..7eb9d4a2506 --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java @@ -0,0 +1,64 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.data.model; + +import inet.ipaddr.AddressStringException; +import inet.ipaddr.IPAddress; +import inet.ipaddr.IPAddressString; +import inet.ipaddr.IPAddressStringParameters; +import org.opensearch.sql.data.type.ExprCoreType; +import org.opensearch.sql.data.type.ExprType; +import org.opensearch.sql.exception.SemanticCheckException; + +/** Expression IP Address Value. */ +public class ExprIpValue extends AbstractExprValue { + private final IPAddress value; + + private static final IPAddressStringParameters validationOptions = + new IPAddressStringParameters.Builder() + .allowEmpty(false) + .allowMask(false) + .allowPrefix(false) + .setEmptyAsLoopback(false) + .allow_inet_aton(false) + .allowSingleSegment(false) + .toParams(); + + public ExprIpValue(String s) { + try { + IPAddress address = new IPAddressString(s, validationOptions).toAddress(); + value = 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())); + } + } + + @Override + public String value() { + return value.toCanonicalString(); + } + + @Override + public ExprType type() { + return ExprCoreType.IP; + } + + @Override + public int compare(ExprValue other) { + return value.compareTo(((ExprIpValue) other).value); + } + + @Override + public boolean equal(ExprValue other) { + return value.equals(((ExprIpValue) other).value); + } + + @Override + public String toString() { + return String.format("IP %s", value()); + } +} diff --git a/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java b/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java index 20813045f20..ed9f0b98efe 100644 --- a/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java +++ b/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java @@ -75,6 +75,10 @@ public static ExprValue timestampValue(Instant value) { return new ExprTimestampValue(value); } + public static ExprValue ipValue(String value) { + return new ExprIpValue(value); + } + /** {@link ExprTupleValue} constructor. */ public static ExprValue tupleValue(Map map) { LinkedHashMap valueMap = new LinkedHashMap<>(); diff --git a/core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java b/core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java index cbc0c982555..6df2ba63905 100644 --- a/core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java +++ b/core/src/main/java/org/opensearch/sql/data/type/ExprCoreType.java @@ -45,6 +45,9 @@ public enum ExprCoreType implements ExprType { TIMESTAMP(STRING, DATE, TIME), INTERVAL(UNDEFINED), + /** IP Address. */ + IP(STRING), + /** Struct. */ STRUCT(UNDEFINED), diff --git a/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java b/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java index d6cb0544d89..3f4752aa2e3 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java +++ b/core/src/test/java/org/opensearch/sql/analysis/AnalyzerTest.java @@ -163,7 +163,7 @@ public void filter_relation_with_invalid_qualifiedName_ExpressionEvaluationExcep assertEquals( "= function expected {[BYTE,BYTE],[SHORT,SHORT],[INTEGER,INTEGER],[LONG,LONG]," + "[FLOAT,FLOAT],[DOUBLE,DOUBLE],[STRING,STRING],[BOOLEAN,BOOLEAN],[DATE,DATE]," - + "[TIME,TIME],[TIMESTAMP,TIMESTAMP],[INTERVAL,INTERVAL]," + + "[TIME,TIME],[TIMESTAMP,TIMESTAMP],[INTERVAL,INTERVAL],[IP,IP]," + "[STRUCT,STRUCT],[ARRAY,ARRAY]}, but got [STRING,INTEGER]", exception.getMessage()); } 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 new file mode 100644 index 00000000000..e985937df5d --- /dev/null +++ b/core/src/test/java/org/opensearch/sql/data/model/ExprIpValueTest.java @@ -0,0 +1,106 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.data.model; + +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 inet.ipaddr.AddressStringException; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.opensearch.sql.data.type.ExprCoreType; +import org.opensearch.sql.exception.SemanticCheckException; + +public class ExprIpValueTest { + + private static final String ipv4String = "1.2.3.4"; + private static final String ipv6String = "2001:db7::ff00:42:8329"; + private static final String ipInvalidString = "INVALID"; + + private static final ExprValue exprIpv4Value = ExprValueUtils.ipValue(ipv4String); + private static final ExprValue exprIpv6Value = ExprValueUtils.ipValue(ipv6String); + + private static final List ipv4LesserStrings = + List.of("1.2.3.3", "01.2.3.3", "::ffff:1.2.3.3", "::ffff:102:303"); + private static final List ipv4EqualStrings = + List.of("1.2.3.4", "01.2.3.4", "::ffff:1.2.3.4", "::ffff:102:304"); + private static final List ipv4GreaterStrings = + List.of("1.2.3.5", "01.2.3.5", "::ffff:1.2.3.5", "::ffff:102:305"); + + private static final List ipv6LesserStrings = + List.of( + "2001:db7::ff00:42:8328", + "2001:0db7::ff00:0042:8328", + "2001:DB7::FF00:42:8328", + "2001:0db7:0000:0000:0000:ff00:0042:8328"); + private static final List ipv6EqualStrings = + List.of( + "2001:db7::ff00:42:8329", + "2001:0db7::ff00:0042:8329", + "2001:DB7::FF00:42:8329", + "2001:0db7:0000:0000:0000:ff00:0042:8329"); + private static final List ipv6GreaterStrings = + List.of( + "2001:db7::ff00:42:8330", + "2001:0db7::ff00:0042:8330", + "2001:DB7::FF00:42:8330", + "2001:0db7:0000:0000:0000:ff00:0042:8330"); + + @Test + public void testInvalid() { + Exception ex = + assertThrows(SemanticCheckException.class, () -> ExprValueUtils.ipValue(ipInvalidString)); + assertTrue( + ex.getMessage() + .matches( + String.format("IP address '%s' is not valid. Error details: .*", ipInvalidString))); + } + + @Test + public void testValue() throws AddressStringException { + ipv4EqualStrings.forEach((s) -> assertEquals(ipv4String, ExprValueUtils.ipValue(s).value())); + ipv6EqualStrings.forEach((s) -> assertEquals(ipv6String, ExprValueUtils.ipValue(s).value())); + } + + @Test + public void testType() { + assertEquals(ExprCoreType.IP, exprIpv4Value.type()); + assertEquals(ExprCoreType.IP, exprIpv6Value.type()); + } + + @Test + public void testCompare() { + ipv4LesserStrings.forEach( + (s) -> assertTrue(exprIpv4Value.compareTo(ExprValueUtils.ipValue(s)) > 0)); + ipv4EqualStrings.forEach( + (s) -> assertEquals(0, exprIpv4Value.compareTo(ExprValueUtils.ipValue(s)))); + ipv4GreaterStrings.forEach( + (s) -> assertTrue(exprIpv4Value.compareTo(ExprValueUtils.ipValue(s)) < 0)); + ipv6LesserStrings.forEach( + (s) -> assertTrue(exprIpv6Value.compareTo(ExprValueUtils.ipValue(s)) > 0)); + ipv6EqualStrings.forEach( + (s) -> assertEquals(0, exprIpv6Value.compareTo(ExprValueUtils.ipValue(s)))); + ipv6GreaterStrings.forEach( + (s) -> assertTrue(exprIpv6Value.compareTo(ExprValueUtils.ipValue(s)) < 0)); + } + + @Test + public void testEquals() { + ipv4EqualStrings.forEach((s) -> assertEquals(exprIpv4Value, ExprValueUtils.ipValue(s))); + ipv6EqualStrings.forEach((s) -> assertEquals(exprIpv6Value, ExprValueUtils.ipValue(s))); + } + + @Test + public void testToString() { + ipv4EqualStrings.forEach( + (s) -> + assertEquals(String.format("IP %s", ipv4String), ExprValueUtils.ipValue(s).toString())); + ipv6EqualStrings.forEach( + (s) -> + assertEquals(String.format("IP %s", ipv6String), ExprValueUtils.ipValue(s).toString())); + } +} diff --git a/core/src/test/java/org/opensearch/sql/expression/function/WideningTypeRuleTest.java b/core/src/test/java/org/opensearch/sql/expression/function/WideningTypeRuleTest.java index 3064ffcdee5..d38be4c958d 100644 --- a/core/src/test/java/org/opensearch/sql/expression/function/WideningTypeRuleTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/function/WideningTypeRuleTest.java @@ -13,6 +13,7 @@ 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.IP; import static org.opensearch.sql.data.type.ExprCoreType.LONG; import static org.opensearch.sql.data.type.ExprCoreType.SHORT; import static org.opensearch.sql.data.type.ExprCoreType.STRING; @@ -57,6 +58,7 @@ class WideningTypeRuleTest { .put(STRING, TIMESTAMP, 1) .put(STRING, DATE, 1) .put(STRING, TIME, 1) + .put(STRING, IP, 1) .put(DATE, TIMESTAMP, 1) .put(TIME, TIMESTAMP, 1) .put(UNDEFINED, BYTE, 1) 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 index c35eacfc721..2bcad9576a5 100644 --- 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 @@ -26,7 +26,7 @@ public enum MappingType { Invalid(null, ExprCoreType.UNKNOWN), Text("text", ExprCoreType.UNKNOWN), Keyword("keyword", ExprCoreType.STRING), - Ip("ip", ExprCoreType.UNKNOWN), + Ip("ip", ExprCoreType.IP), GeoPoint("geo_point", ExprCoreType.UNKNOWN), Binary("binary", ExprCoreType.UNKNOWN), Date("date", ExprCoreType.TIMESTAMP), 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 index 22581ec28c4..e7e90ae815d 100644 --- 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 @@ -5,7 +5,7 @@ package org.opensearch.sql.opensearch.data.type; -import static org.opensearch.sql.data.type.ExprCoreType.UNKNOWN; +import static org.opensearch.sql.data.type.ExprCoreType.IP; import lombok.EqualsAndHashCode; @@ -20,7 +20,7 @@ public class OpenSearchIpType extends OpenSearchDataType { private OpenSearchIpType() { super(MappingType.Ip); - exprCoreType = UNKNOWN; + exprCoreType = IP; } public static OpenSearchIpType of() { diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprIpValue.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprIpValue.java deleted file mode 100644 index 30b3784bfcf..00000000000 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprIpValue.java +++ /dev/null @@ -1,48 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.opensearch.data.value; - -import java.util.Objects; -import lombok.RequiredArgsConstructor; -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.OpenSearchIpType; - -/** - * OpenSearch IP ExprValue
- * Todo, add this to avoid the unknown value type exception, the implementation will be changed. - */ -@RequiredArgsConstructor -public class OpenSearchExprIpValue extends AbstractExprValue { - - private final String ip; - - @Override - public Object value() { - return ip; - } - - @Override - public ExprType type() { - return OpenSearchIpType.of(); - } - - @Override - public int compare(ExprValue other) { - return ip.compareTo(((OpenSearchExprIpValue) other).ip); - } - - @Override - public boolean equal(ExprValue other) { - return ip.equals(((OpenSearchExprIpValue) other).ip); - } - - @Override - public int hashCode() { - return Objects.hashCode(ip); - } -} diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java index 41d6667ded4..401f270f075 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java @@ -50,6 +50,7 @@ import org.opensearch.sql.data.model.ExprDoubleValue; import org.opensearch.sql.data.model.ExprFloatValue; import org.opensearch.sql.data.model.ExprIntegerValue; +import org.opensearch.sql.data.model.ExprIpValue; import org.opensearch.sql.data.model.ExprLongValue; import org.opensearch.sql.data.model.ExprNullValue; import org.opensearch.sql.data.model.ExprShortValue; @@ -133,8 +134,8 @@ public void extendTypeMapping(Map typeMapping) { OpenSearchDateType.of(TIMESTAMP), OpenSearchExprValueFactory::createOpenSearchDateType) .put( - OpenSearchDataType.of(OpenSearchDataType.MappingType.Ip), - (c, dt) -> new OpenSearchExprIpValue(c.stringValue())) + OpenSearchDateType.of(OpenSearchDataType.MappingType.Ip), + (c, dt) -> new ExprIpValue(c.stringValue())) .put( OpenSearchDataType.of(OpenSearchDataType.MappingType.Binary), (c, dt) -> new OpenSearchExprBinaryValue(c.stringValue())) diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeRecognitionTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeRecognitionTest.java index 35ad6b7ea64..2e900045713 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeRecognitionTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeRecognitionTest.java @@ -17,7 +17,6 @@ import org.opensearch.sql.expression.DSL; import org.opensearch.sql.opensearch.data.value.OpenSearchExprBinaryValue; import org.opensearch.sql.opensearch.data.value.OpenSearchExprGeoPointValue; -import org.opensearch.sql.opensearch.data.value.OpenSearchExprIpValue; import org.opensearch.sql.opensearch.data.value.OpenSearchExprTextValue; public class OpenSearchDataTypeRecognitionTest { @@ -33,7 +32,6 @@ private static Stream types() { return Stream.of( Arguments.of("TEXT", new OpenSearchExprTextValue("A"), "text without fields"), Arguments.of("BINARY", new OpenSearchExprBinaryValue("A"), "binary"), - Arguments.of("IP", new OpenSearchExprIpValue("A"), "ip"), Arguments.of("TEXT", new TestTextWithFieldValue("Hello World"), "text with fields"), Arguments.of("GEO_POINT", new OpenSearchExprGeoPointValue(0d, 0d), "geo point")); } 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 index 76fbbd6e65f..77b905e228c 100644 --- 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 @@ -22,6 +22,7 @@ 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.IP; import static org.opensearch.sql.data.type.ExprCoreType.LONG; import static org.opensearch.sql.data.type.ExprCoreType.SHORT; import static org.opensearch.sql.data.type.ExprCoreType.STRING; @@ -108,9 +109,9 @@ private static Stream getTestDataWithType() { Arguments.of(MappingType.DateNanos, "timestamp", TIMESTAMP), Arguments.of(MappingType.Object, "object", STRUCT), Arguments.of(MappingType.Nested, "nested", ARRAY), + Arguments.of(MappingType.Ip, "ip", IP), Arguments.of(MappingType.GeoPoint, "geo_point", OpenSearchGeoPointType.of()), - Arguments.of(MappingType.Binary, "binary", OpenSearchBinaryType.of()), - Arguments.of(MappingType.Ip, "ip", OpenSearchIpType.of())); + Arguments.of(MappingType.Binary, "binary", OpenSearchBinaryType.of())); } @ParameterizedTest(name = "{1}") @@ -188,13 +189,13 @@ public void types_but_clones_are_singletons_and_cached() { () -> assertSame(OpenSearchDataType.of(MappingType.Text), OpenSearchTextType.of()), () -> assertSame(OpenSearchDataType.of(MappingType.Binary), OpenSearchBinaryType.of()), () -> assertSame(OpenSearchDataType.of(MappingType.GeoPoint), OpenSearchGeoPointType.of()), - () -> assertSame(OpenSearchDataType.of(MappingType.Ip), OpenSearchIpType.of()), () -> assertNotSame( OpenSearchTextType.of(), OpenSearchTextType.of(Map.of("properties", OpenSearchDataType.of(INTEGER)))), () -> assertSame(OpenSearchDataType.of(INTEGER), OpenSearchDataType.of(INTEGER)), () -> assertSame(OpenSearchDataType.of(STRING), OpenSearchDataType.of(STRING)), + () -> assertSame(OpenSearchDataType.of(IP), OpenSearchDataType.of(IP)), () -> assertSame(OpenSearchDataType.of(STRUCT), OpenSearchDataType.of(STRUCT)), () -> assertNotSame( diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprIpValueTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprIpValueTest.java index 5ee175f3043..8b137891791 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprIpValueTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprIpValueTest.java @@ -1,45 +1 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ -package org.opensearch.sql.opensearch.data.value; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertTrue; - -import org.junit.jupiter.api.Test; -import org.opensearch.sql.opensearch.data.type.OpenSearchIpType; - -public class OpenSearchExprIpValueTest { - - private final String ipString = "192.168.0.1"; - private final OpenSearchExprIpValue ipValue = new OpenSearchExprIpValue(ipString); - - @Test - void testValue() { - assertEquals(ipString, ipValue.value()); - } - - @Test - void testType() { - assertEquals(OpenSearchIpType.of(), ipValue.type()); - } - - @Test - void testCompare() { - assertEquals(0, ipValue.compareTo(new OpenSearchExprIpValue(ipString))); - assertEquals(ipValue, new OpenSearchExprIpValue(ipString)); - } - - @Test - void testEqual() { - assertTrue(ipValue.equal(new OpenSearchExprIpValue(ipString))); - } - - @Test - void testHashCode() { - assertNotNull(ipValue.hashCode()); - } -} diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java index d82926077e2..90efba3a64e 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java @@ -16,6 +16,7 @@ import static org.opensearch.sql.data.model.ExprValueUtils.doubleValue; import static org.opensearch.sql.data.model.ExprValueUtils.floatValue; import static org.opensearch.sql.data.model.ExprValueUtils.integerValue; +import static org.opensearch.sql.data.model.ExprValueUtils.ipValue; import static org.opensearch.sql.data.model.ExprValueUtils.longValue; import static org.opensearch.sql.data.model.ExprValueUtils.nullValue; import static org.opensearch.sql.data.model.ExprValueUtils.shortValue; @@ -51,6 +52,7 @@ import org.opensearch.geometry.utils.Geohash; import org.opensearch.sql.data.model.ExprCollectionValue; import org.opensearch.sql.data.model.ExprDateValue; +import org.opensearch.sql.data.model.ExprIpValue; import org.opensearch.sql.data.model.ExprTimeValue; import org.opensearch.sql.data.model.ExprTimestampValue; import org.opensearch.sql.data.model.ExprTupleValue; @@ -118,9 +120,10 @@ class OpenSearchExprValueFactoryTest { .put("geoV", OpenSearchDataType.of(OpenSearchDataType.MappingType.GeoPoint)) .put("binaryV", OpenSearchDataType.of(OpenSearchDataType.MappingType.Binary)) .build(); - private static final double TOLERANCE = 1E-5; + private final OpenSearchExprValueFactory exprValueFactory = new OpenSearchExprValueFactory(MAPPING, true); + private final OpenSearchExprValueFactory exprValueFactoryNoArrays = new OpenSearchExprValueFactory(MAPPING, false); @@ -215,6 +218,16 @@ public void constructString() { () -> assertEquals(stringValue("text"), constructFromObject("stringV", "text"))); } + @Test + public void constructIp() { + assertAll( + () -> assertEquals(ipValue("1.2.3.4"), tupleValue("{\"ipV\":\"1.2.3.4\"}").get("ipV")), + () -> + assertEquals( + ipValue("2001:db7::ff00:42:8329"), + constructFromObject("ipV", "2001:db7::ff00:42:8329"))); + } + @Test public void constructBoolean() { assertAll( @@ -659,17 +672,6 @@ public void constructArrayOfGeoPointsReturnsAll() { .get("geoV")); } - @Test - public void constructArrayOfIPsReturnsAll() { - final String ip1 = "192.168.0.1"; - final String ip2 = "192.168.0.2"; - - assertEquals( - new ExprCollectionValue( - List.of(new OpenSearchExprIpValue(ip1), new OpenSearchExprIpValue(ip2))), - tupleValue(String.format("{\"%s\":[\"%s\",\"%s\"]}", fieldIp, ip1, ip2)).get(fieldIp)); - } - @Test public void constructBinaryArrayReturnsAll() { assertEquals( @@ -681,6 +683,16 @@ public void constructBinaryArrayReturnsAll() { .get("binaryV")); } + @Test + public void constructArrayOfIPsReturnsAll() { + final String ipv4String = "1.2.3.4"; + final String ipv6String = "2001:db7::ff00:42:8329"; + assertEquals( + new ExprCollectionValue(List.of(ipValue(ipv4String), ipValue(ipv6String))), + tupleValue(String.format("{\"%s\":[\"%s\",\"%s\"]}", fieldIp, ipv4String, ipv6String)) + .get(fieldIp)); + } + @Test public void constructArrayOfCustomEpochMillisReturnsAll() { assertEquals( @@ -743,12 +755,14 @@ public void constructStruct() { @Test public void constructIP() { - final String valueIp = "192.168.0.1"; + final String ipString = "192.168.0.1"; assertEquals( - new OpenSearchExprIpValue(valueIp), - tupleValue(String.format("{\"%s\":\"%s\"}", fieldIp, valueIp)).get(fieldIp)); + new ExprIpValue(ipString), + tupleValue(String.format("{\"%s\":\"%s\"}", fieldIp, ipString)).get(fieldIp)); } + private static final double TOLERANCE = 1E-5; + @Test public void constructGeoPoint() { final double lat = 42.60355556; From 19b91006a933a60a7c30c5818e6f38b97853b9d3 Mon Sep 17 00:00:00 2001 From: currantw Date: Thu, 28 Nov 2024 12:02:50 -0800 Subject: [PATCH 02/26] Add support for casting (`cast(field_name to ip)`) and remove existing unused sorting syntax. Signed-off-by: currantw --- .../org/opensearch/sql/ast/dsl/AstDSL.java | 2 +- .../opensearch/sql/ast/expression/Cast.java | 2 + .../sql/data/model/ExprIpValue.java | 6 +- .../org/opensearch/sql/expression/DSL.java | 4 ++ .../function/BuiltinFunctionName.java | 1 + .../operator/convert/TypeCastOperators.java | 10 +++ .../convert/TypeCastOperatorTest.java | 67 +++++++++++++++++-- .../script/filter/lucene/LuceneQuery.java | 9 +++ .../script/filter/FilterQueryBuilderTest.java | 23 +++++++ ppl/src/main/antlr/OpenSearchPPLLexer.g4 | 7 +- ppl/src/main/antlr/OpenSearchPPLParser.g4 | 15 +---- .../sql/ppl/parser/AstExpressionBuilder.java | 3 +- .../sql/ppl/utils/ArgumentFactory.java | 13 +--- .../sql/ppl/parser/AstBuilderTest.java | 5 +- .../ppl/parser/AstExpressionBuilderTest.java | 60 +++-------------- .../sql/ppl/utils/ArgumentFactoryTest.java | 21 +++--- 16 files changed, 138 insertions(+), 110 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java b/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java index d9956609ecb..62752efe52f 100644 --- a/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java +++ b/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java @@ -431,7 +431,7 @@ public static List sortOptions() { } public static List defaultSortFieldArgs() { - return exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral())); + return exprList(argument("asc", booleanLiteral(true))); } public static Span span(UnresolvedExpression field, UnresolvedExpression value, SpanUnit unit) { diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/Cast.java b/core/src/main/java/org/opensearch/sql/ast/expression/Cast.java index 2019346fb52..541dbedeadd 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/Cast.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/Cast.java @@ -12,6 +12,7 @@ import static org.opensearch.sql.expression.function.BuiltinFunctionName.CAST_TO_DOUBLE; import static org.opensearch.sql.expression.function.BuiltinFunctionName.CAST_TO_FLOAT; import static org.opensearch.sql.expression.function.BuiltinFunctionName.CAST_TO_INT; +import static org.opensearch.sql.expression.function.BuiltinFunctionName.CAST_TO_IP; import static org.opensearch.sql.expression.function.BuiltinFunctionName.CAST_TO_LONG; import static org.opensearch.sql.expression.function.BuiltinFunctionName.CAST_TO_SHORT; import static org.opensearch.sql.expression.function.BuiltinFunctionName.CAST_TO_STRING; @@ -54,6 +55,7 @@ public class Cast extends UnresolvedExpression { .put("time", CAST_TO_TIME.getName()) .put("timestamp", CAST_TO_TIMESTAMP.getName()) .put("datetime", CAST_TO_DATETIME.getName()) + .put("ip", CAST_TO_IP.getName()) .build(); /** The source expression cast from. */ 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 index 7eb9d4a2506..71b3ad8eb50 100644 --- a/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java +++ b/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java @@ -49,12 +49,14 @@ public ExprType type() { @Override public int compare(ExprValue other) { - return value.compareTo(((ExprIpValue) other).value); + IPAddress otherValue = ((ExprIpValue) other).value; + return value.compareTo(otherValue); } @Override public boolean equal(ExprValue other) { - return value.equals(((ExprIpValue) other).value); + IPAddress otherValue = ((ExprIpValue) other).value; + return value.equals(otherValue); } @Override diff --git a/core/src/main/java/org/opensearch/sql/expression/DSL.java b/core/src/main/java/org/opensearch/sql/expression/DSL.java index 54bd35e70fa..44ecc2bc867 100644 --- a/core/src/main/java/org/opensearch/sql/expression/DSL.java +++ b/core/src/main/java/org/opensearch/sql/expression/DSL.java @@ -835,6 +835,10 @@ public static FunctionExpression castTimestamp(Expression value) { return compile(FunctionProperties.None, BuiltinFunctionName.CAST_TO_TIMESTAMP, value); } + public static FunctionExpression castIp(Expression value) { + return compile(FunctionProperties.None, BuiltinFunctionName.CAST_TO_IP, value); + } + public static FunctionExpression typeof(Expression value) { return compile(FunctionProperties.None, BuiltinFunctionName.TYPEOF, value); } diff --git a/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java b/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java index a67308c96ab..f8e9cf7c5f7 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java @@ -231,6 +231,7 @@ public enum BuiltinFunctionName { CAST_TO_TIME(FunctionName.of("cast_to_time")), CAST_TO_TIMESTAMP(FunctionName.of("cast_to_timestamp")), CAST_TO_DATETIME(FunctionName.of("cast_to_datetime")), + CAST_TO_IP(FunctionName.of("cast_to_ip")), TYPEOF(FunctionName.of("typeof")), /** Relevance Function. */ diff --git a/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperators.java b/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperators.java index 55e223d94cd..696d02dc0f8 100644 --- a/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperators.java +++ b/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperators.java @@ -11,6 +11,7 @@ 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.IP; import static org.opensearch.sql.data.type.ExprCoreType.LONG; import static org.opensearch.sql.data.type.ExprCoreType.SHORT; import static org.opensearch.sql.data.type.ExprCoreType.STRING; @@ -31,6 +32,7 @@ import org.opensearch.sql.data.model.ExprDoubleValue; import org.opensearch.sql.data.model.ExprFloatValue; import org.opensearch.sql.data.model.ExprIntegerValue; +import org.opensearch.sql.data.model.ExprIpValue; import org.opensearch.sql.data.model.ExprLongValue; import org.opensearch.sql.data.model.ExprShortValue; import org.opensearch.sql.data.model.ExprStringValue; @@ -54,6 +56,7 @@ public static void register(BuiltinFunctionRepository repository) { repository.register(castToFloat()); repository.register(castToDouble()); repository.register(castToBoolean()); + repository.register(castToIp()); repository.register(castToDate()); repository.register(castToTime()); repository.register(castToTimestamp()); @@ -173,6 +176,13 @@ private static DefaultFunctionResolver castToBoolean() { impl(nullMissingHandling((v) -> v), BOOLEAN, BOOLEAN)); } + private static DefaultFunctionResolver castToIp() { + return FunctionDSL.define( + BuiltinFunctionName.CAST_TO_IP.getName(), + impl(nullMissingHandling((v) -> v), IP, IP), + impl(nullMissingHandling((v) -> new ExprIpValue(v.stringValue())), IP, STRING)); + } + private static DefaultFunctionResolver castToDate() { return FunctionDSL.define( BuiltinFunctionName.CAST_TO_DATE.getName(), 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 index 44a3ccabbd4..0b025b91ee2 100644 --- 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 @@ -7,12 +7,14 @@ 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.opensearch.sql.data.type.ExprCoreType.BOOLEAN; import static org.opensearch.sql.data.type.ExprCoreType.BYTE; import static org.opensearch.sql.data.type.ExprCoreType.DATE; 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.IP; import static org.opensearch.sql.data.type.ExprCoreType.LONG; import static org.opensearch.sql.data.type.ExprCoreType.SHORT; import static org.opensearch.sql.data.type.ExprCoreType.STRING; @@ -29,12 +31,17 @@ import org.opensearch.sql.data.model.ExprDoubleValue; import org.opensearch.sql.data.model.ExprFloatValue; import org.opensearch.sql.data.model.ExprIntegerValue; +import org.opensearch.sql.data.model.ExprIpValue; import org.opensearch.sql.data.model.ExprLongValue; +import org.opensearch.sql.data.model.ExprMissingValue; +import org.opensearch.sql.data.model.ExprNullValue; import org.opensearch.sql.data.model.ExprShortValue; import org.opensearch.sql.data.model.ExprStringValue; import org.opensearch.sql.data.model.ExprTimeValue; import org.opensearch.sql.data.model.ExprTimestampValue; import org.opensearch.sql.data.model.ExprValue; +import org.opensearch.sql.exception.ExpressionEvaluationException; +import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.expression.DSL; import org.opensearch.sql.expression.FunctionExpression; @@ -316,10 +323,6 @@ void castToTime() { assertEquals(TIME, expression.type()); assertEquals(new ExprTimeValue("01:01:01"), expression.valueOf()); - expression = DSL.castTime(DSL.literal(new ExprTimestampValue("2012-08-07 01:01:01"))); - assertEquals(TIME, expression.type()); - assertEquals(new ExprTimeValue("01:01:01"), expression.valueOf()); - expression = DSL.castTime(DSL.literal(new ExprTimeValue("01:01:01"))); assertEquals(TIME, expression.type()); assertEquals(new ExprTimeValue("01:01:01"), expression.valueOf()); @@ -334,9 +337,59 @@ void castToTimestamp() { expression = DSL.castTimestamp(DSL.literal(new ExprTimestampValue("2012-08-07 01:01:01"))); assertEquals(TIMESTAMP, expression.type()); assertEquals(new ExprTimestampValue("2012-08-07 01:01:01"), expression.valueOf()); + } - expression = DSL.castTimestamp(DSL.literal(new ExprTimestampValue("2012-08-07 01:01:01"))); - assertEquals(TIMESTAMP, expression.type()); - assertEquals(new ExprTimestampValue("2012-08-07 01:01:01"), expression.valueOf()); + @Test + void castToIp() { + FunctionExpression exp; + + String expectedMsg; + String actualMsg; + + final String ipv4String = "1.2.3.4"; + final String ipv6String = "2001:db7::ff00:42:8329"; + final String ipInvalidString = "INVALID"; + + final ExprValue exprIpv4Value = new ExprIpValue(ipv4String); + final ExprValue exprIpv6Value = new ExprIpValue(ipv6String); + + // From string + exp = DSL.castIp(DSL.literal(ipv4String)); + assertEquals(IP, exp.type()); + assertEquals(exprIpv4Value, exp.valueOf()); + + exp = DSL.castIp(DSL.literal(ipv6String)); + assertEquals(IP, exp.type()); + assertEquals(exprIpv6Value, exp.valueOf()); + + 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); + assertTrue(actualMsg.matches(expectedMsg)); + + // From IP address + exp = DSL.castIp(DSL.literal(exprIpv4Value)); + assertEquals(IP, exp.type()); + assertEquals(exprIpv4Value, exp.valueOf()); + + exp = DSL.castIp(DSL.literal(exprIpv6Value)); + assertEquals(IP, exp.type()); + assertEquals(exprIpv6Value, exp.valueOf()); + + // From invalid type + actualMsg = + assertThrows(ExpressionEvaluationException.class, () -> DSL.castIp(DSL.literal(0))) + .getMessage(); + expectedMsg = "cast_to_ip function expected {[IP],[STRING]}, but get [INTEGER]"; + assertEquals(expectedMsg, actualMsg); + + // From null or missing value + exp = DSL.castIp(DSL.literal(ExprNullValue.of())); + assertEquals(IP, exp.type()); + assertTrue(exp.valueOf().isNull()); + + exp = DSL.castIp(DSL.literal(ExprMissingValue.of())); + assertEquals(IP, exp.type()); + assertTrue(exp.valueOf().isMissing()); } } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java index c9ef5bcca5d..36ebb92253a 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java @@ -18,6 +18,7 @@ import org.opensearch.sql.data.model.ExprDoubleValue; import org.opensearch.sql.data.model.ExprFloatValue; import org.opensearch.sql.data.model.ExprIntegerValue; +import org.opensearch.sql.data.model.ExprIpValue; import org.opensearch.sql.data.model.ExprLongValue; import org.opensearch.sql.data.model.ExprShortValue; import org.opensearch.sql.data.model.ExprStringValue; @@ -211,6 +212,14 @@ private ExprValue cast(FunctionExpression castFunction, ReferenceExpression ref) return expr.valueOf(); } }) + .put( + BuiltinFunctionName.CAST_TO_IP.getName(), + (expr, ref) -> { + ExprValue value = expr.valueOf(); + return value.type().equals(ExprCoreType.IP) + ? value + : new ExprIpValue(value.stringValue()); + }) .put( BuiltinFunctionName.CAST_TO_DATE.getName(), (expr, ref) -> { diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java index bd2a9901ed9..bc07ebedcc1 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java @@ -16,6 +16,7 @@ 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.IP; import static org.opensearch.sql.data.type.ExprCoreType.LONG; import static org.opensearch.sql.data.type.ExprCoreType.SHORT; import static org.opensearch.sql.data.type.ExprCoreType.STRING; @@ -59,6 +60,10 @@ @ExtendWith(MockitoExtension.class) class FilterQueryBuilderTest { + private static Stream ipCastSource() { + return Stream.of(literal("1.2.3.4"), literal("2001:db7::ff00:42:8329")); + } + private static Stream numericCastSource() { return Stream.of( literal((byte) 1), @@ -1715,6 +1720,24 @@ void cast_to_boolean_false_in_filter(LiteralExpression expr) { json, buildQuery(DSL.equal(ref("boolean_value", BOOLEAN), DSL.castBoolean(expr)))); } + @ParameterizedTest(name = "castIp({0})") + @MethodSource({"ipCastSource"}) + void cast_to_ip_in_filter(LiteralExpression expr) { + String json = + String.format( + "{\n" + + " \"term\" : {\n" + + " \"ip_value\" : {\n" + + " \"value\" : \"%s\",\n" + + " \"boost\" : 1.0\n" + + " }\n" + + " }\n" + + "}", + expr.valueOf().stringValue()); + + assertJsonEquals(json, buildQuery(DSL.equal(ref("ip_value", IP), DSL.castIp(expr)))); + } + @Test void cast_from_boolean() { Expression booleanExpr = literal(false); diff --git a/ppl/src/main/antlr/OpenSearchPPLLexer.g4 b/ppl/src/main/antlr/OpenSearchPPLLexer.g4 index 4a883fa6563..3b2fce4f1c6 100644 --- a/ppl/src/main/antlr/OpenSearchPPLLexer.g4 +++ b/ppl/src/main/antlr/OpenSearchPPLLexer.g4 @@ -52,12 +52,6 @@ WITH: 'WITH'; // CLAUSE KEYWORDS SORTBY: 'SORTBY'; -// FIELD KEYWORDS -AUTO: 'AUTO'; -STR: 'STR'; -IP: 'IP'; -NUM: 'NUM'; - // TRENDLINE KEYWORDS SMA: 'SMA'; @@ -142,6 +136,7 @@ LONG: 'LONG'; FLOAT: 'FLOAT'; STRING: 'STRING'; BOOLEAN: 'BOOLEAN'; +IP: 'IP'; // SPECIAL CHARACTERS AND OPERATORS PIPE: '|'; diff --git a/ppl/src/main/antlr/OpenSearchPPLParser.g4 b/ppl/src/main/antlr/OpenSearchPPLParser.g4 index c9d0f2e110e..fd18b06aa64 100644 --- a/ppl/src/main/antlr/OpenSearchPPLParser.g4 +++ b/ppl/src/main/antlr/OpenSearchPPLParser.g4 @@ -363,15 +363,7 @@ wcFieldList ; sortField - : (PLUS | MINUS)? sortFieldExpression - ; - -sortFieldExpression - : fieldExpression - | AUTO LT_PRTHS fieldExpression RT_PRTHS - | STR LT_PRTHS fieldExpression RT_PRTHS - | IP LT_PRTHS fieldExpression RT_PRTHS - | NUM LT_PRTHS fieldExpression RT_PRTHS + : (PLUS | MINUS)? fieldExpression ; fieldExpression @@ -408,6 +400,7 @@ convertedDataType | typeName = FLOAT | typeName = STRING | typeName = BOOLEAN + | typeName = IP ; evalFunctionName @@ -897,10 +890,6 @@ keywordsCanBeId | DATASOURCES // CLAUSEKEYWORDS | SORTBY - // FIELDKEYWORDSAUTO - | STR - | IP - | NUM // ARGUMENT KEYWORDS | KEEPEMPTY | CONSECUTIVE diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java index 8bc98c8eee6..d18d74e40a3 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java @@ -162,8 +162,7 @@ public UnresolvedExpression visitWcFieldExpression(WcFieldExpressionContext ctx) @Override public UnresolvedExpression visitSortField(SortFieldContext ctx) { return new Field( - visit(ctx.sortFieldExpression().fieldExpression().qualifiedName()), - ArgumentFactory.getArgumentList(ctx)); + visit(ctx.fieldExpression().qualifiedName()), ArgumentFactory.getArgumentList(ctx)); } /** Aggregation function. */ diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java b/ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java index f89ecf9c6e4..cd79ccef79f 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java @@ -87,19 +87,10 @@ public static List getArgumentList(DedupCommandContext ctx) { * @return the list of arguments fetched from the sort field in sort command */ public static List getArgumentList(SortFieldContext ctx) { - return Arrays.asList( + return List.of( ctx.MINUS() != null ? new Argument("asc", new Literal(false, DataType.BOOLEAN)) - : new Argument("asc", new Literal(true, DataType.BOOLEAN)), - ctx.sortFieldExpression().AUTO() != null - ? new Argument("type", new Literal("auto", DataType.STRING)) - : ctx.sortFieldExpression().IP() != null - ? new Argument("type", new Literal("ip", DataType.STRING)) - : ctx.sortFieldExpression().NUM() != null - ? new Argument("type", new Literal("num", DataType.STRING)) - : ctx.sortFieldExpression().STR() != null - ? new Argument("type", new Literal("str", DataType.STRING)) - : new Argument("type", new Literal(null, DataType.NULL))); + : new Argument("asc", new Literal(true, DataType.BOOLEAN))); } /** diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java index c6f4ed20447..adabd045051 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java @@ -29,7 +29,6 @@ import static org.opensearch.sql.ast.dsl.AstDSL.intLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.let; import static org.opensearch.sql.ast.dsl.AstDSL.map; -import static org.opensearch.sql.ast.dsl.AstDSL.nullLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.parse; import static org.opensearch.sql.ast.dsl.AstDSL.projectWithArg; import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName; @@ -433,9 +432,7 @@ public void testSortCommandWithOptions() { "source=t | sort - f1, + f2", sort( relation("t"), - field( - "f1", - exprList(argument("asc", booleanLiteral(false)), argument("type", nullLiteral()))), + field("f1", exprList(argument("asc", booleanLiteral(false)))), field("f2", defaultSortFieldArgs()))); } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java index fbb25549ab8..e4cee770136 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java @@ -32,7 +32,6 @@ import static org.opensearch.sql.ast.dsl.AstDSL.let; import static org.opensearch.sql.ast.dsl.AstDSL.longLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.not; -import static org.opensearch.sql.ast.dsl.AstDSL.nullLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.or; import static org.opensearch.sql.ast.dsl.AstDSL.projectWithArg; import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName; @@ -326,65 +325,22 @@ public void testFieldExpr() { } @Test - public void testSortFieldWithMinusKeyword() { + public void testSortFieldWithPlusKeyword() { assertEqual( - "source=t | sort - f", - sort( - relation("t"), - field("f", argument("asc", booleanLiteral(false)), argument("type", nullLiteral())))); - } - - @Test - public void testSortFieldWithBackticks() { - assertEqual("source=t | sort `f`", sort(relation("t"), field("f", defaultSortFieldArgs()))); - } - - @Test - public void testSortFieldWithAutoKeyword() { - assertEqual( - "source=t | sort auto(f)", - sort( - relation("t"), - field( - "f", - argument("asc", booleanLiteral(true)), - argument("type", stringLiteral("auto"))))); + "source=t | sort + f", + sort(relation("t"), field("f", argument("asc", booleanLiteral(true))))); } @Test - public void testSortFieldWithIpKeyword() { - assertEqual( - "source=t | sort ip(f)", - sort( - relation("t"), - field( - "f", - argument("asc", booleanLiteral(true)), - argument("type", stringLiteral("ip"))))); - } - - @Test - public void testSortFieldWithNumKeyword() { + public void testSortFieldWithMinusKeyword() { assertEqual( - "source=t | sort num(f)", - sort( - relation("t"), - field( - "f", - argument("asc", booleanLiteral(true)), - argument("type", stringLiteral("num"))))); + "source=t | sort - f", + sort(relation("t"), field("f", argument("asc", booleanLiteral(false))))); } @Test - public void testSortFieldWithStrKeyword() { - assertEqual( - "source=t | sort str(f)", - sort( - relation("t"), - field( - "f", - argument("asc", booleanLiteral(true)), - argument("type", stringLiteral("str"))))); + public void testSortFieldWithBackticks() { + assertEqual("source=t | sort `f`", sort(relation("t"), field("f", defaultSortFieldArgs()))); } @Test diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java index 761dbe2997b..dea5598b9a3 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java @@ -82,20 +82,17 @@ public void testDedupCommandDefaultArgument() { @Test public void testSortCommandDefaultArgument() { - assertEqual("source=t | sort field0", "source=t | sort field0"); - } + assertEqual( + "source=t | sort field0", + sort(relation("t"), field("field0", exprList(argument("asc", booleanLiteral(true)))))); - @Test - public void testSortFieldArgument() { assertEqual( - "source=t | sort - auto(field0)", - sort( - relation("t"), - field( - "field0", - exprList( - argument("asc", booleanLiteral(false)), - argument("type", stringLiteral("auto")))))); + "source=t | sort + field0", + sort(relation("t"), field("field0", exprList(argument("asc", booleanLiteral(true)))))); + + assertEqual( + "source=t | sort - field0", + sort(relation("t"), field("field0", exprList(argument("asc", booleanLiteral(false)))))); } @Test From f842ddb107971b7fc4445aeb8c4c570ed877b39c Mon Sep 17 00:00:00 2001 From: currantw Date: Mon, 2 Dec 2024 10:40:06 -0800 Subject: [PATCH 03/26] Update comparison logic to compare in IPv6 Signed-off-by: currantw --- .../sql/data/model/ExprIpValue.java | 43 ++++++++++++++----- .../script/filter/FilterQueryBuilderTest.java | 36 ++++++++-------- .../sql/ppl/utils/ArgumentFactoryTest.java | 2 +- 3 files changed, 52 insertions(+), 29 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 index 71b3ad8eb50..bbf4bd8c59c 100644 --- a/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java +++ b/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java @@ -9,6 +9,8 @@ import inet.ipaddr.IPAddress; import inet.ipaddr.IPAddressString; import inet.ipaddr.IPAddressStringParameters; +import inet.ipaddr.ipv4.IPv4Address; +import inet.ipaddr.ipv6.IPv6Address; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; import org.opensearch.sql.exception.SemanticCheckException; @@ -28,13 +30,7 @@ public class ExprIpValue extends AbstractExprValue { .toParams(); public ExprIpValue(String s) { - try { - IPAddress address = new IPAddressString(s, validationOptions).toAddress(); - value = 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())); - } + value = stringToIpAddress(s); } @Override @@ -49,18 +45,43 @@ public ExprType type() { @Override public int compare(ExprValue other) { - IPAddress otherValue = ((ExprIpValue) other).value; - return value.compareTo(otherValue); + IPAddress otherValue = + other instanceof ExprIpValue exprIpValue + ? exprIpValue.value + : stringToIpAddress(other.stringValue()); + + // Map IPv4 addresses to IPv6 for comparison + IPv6Address ipv6Value = toIPv6Address(value); + IPv6Address otherIpv6Value = toIPv6Address(otherValue); + + return ipv6Value.compareTo(otherIpv6Value); } @Override public boolean equal(ExprValue other) { - IPAddress otherValue = ((ExprIpValue) other).value; - return value.equals(otherValue); + return compare(other) == 0; } @Override 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 + ? iPv4Address.toIPv6() + : (IPv6Address) ipAddress; + } } diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java index bc07ebedcc1..832c92826a1 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java @@ -1725,14 +1725,15 @@ void cast_to_boolean_false_in_filter(LiteralExpression expr) { void cast_to_ip_in_filter(LiteralExpression expr) { String json = String.format( - "{\n" - + " \"term\" : {\n" - + " \"ip_value\" : {\n" - + " \"value\" : \"%s\",\n" - + " \"boost\" : 1.0\n" - + " }\n" - + " }\n" - + "}", + """ + { + "term" : { + "ip_value" : { + "value" : "%s", + "boost" : 1.0 + } + } + }""", expr.valueOf().stringValue()); assertJsonEquals(json, buildQuery(DSL.equal(ref("ip_value", IP), DSL.castIp(expr)))); @@ -1906,15 +1907,16 @@ void non_literal_in_cast_should_build_script() { void non_cast_nested_function_should_build_script() { mockToStringSerializer(); assertJsonEquals( - "{\n" - + " \"script\" : {\n" - + " \"script\" : {\n" - + " \"source\" : \"=(integer_value, abs(+(1, 0)))\",\n" - + " \"lang\" : \"opensearch_query_expression\"\n" - + " },\n" - + " \"boost\" : 1.0\n" - + " }\n" - + "}", + """ + { + "script" : { + "script" : { + "source" : "=(integer_value, abs(+(1, 0)))", + "lang" : "opensearch_query_expression" + }, + "boost" : 1.0 + } + }""", buildQuery( DSL.equal(ref("integer_value", INTEGER), DSL.abs(DSL.add(literal(1), literal(0)))))); } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java index dea5598b9a3..f7369218778 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java @@ -81,7 +81,7 @@ public void testDedupCommandDefaultArgument() { } @Test - public void testSortCommandDefaultArgument() { + public void testSortCommand() { assertEqual( "source=t | sort field0", sort(relation("t"), field("field0", exprList(argument("asc", booleanLiteral(true)))))); From 16b1825763323ba457240feaab601ef12c0c78c4 Mon Sep 17 00:00:00 2001 From: currantw Date: Mon, 2 Dec 2024 12:17:39 -0800 Subject: [PATCH 04/26] Fix bug casting to IP Signed-off-by: currantw --- .../opensearch/sql/opensearch/data/type/OpenSearchDataType.java | 2 -- 1 file changed, 2 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 index 2bcad9576a5..6c8912be864 100644 --- 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,8 +160,6 @@ public static OpenSearchDataType of(MappingType mappingType, Map 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 From 8e336358d20a421a64c208d04927caa4b85224ef Mon Sep 17 00:00:00 2001 From: currantw Date: Mon, 2 Dec 2024 13:27:37 -0800 Subject: [PATCH 05/26] Fix failing tests Signed-off-by: currantw --- .../sql/expression/operator/convert/TypeCastOperatorTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 0b025b91ee2..80df91b6f52 100644 --- 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 @@ -380,7 +380,7 @@ void castToIp() { actualMsg = assertThrows(ExpressionEvaluationException.class, () -> DSL.castIp(DSL.literal(0))) .getMessage(); - expectedMsg = "cast_to_ip function expected {[IP],[STRING]}, but get [INTEGER]"; + expectedMsg = "cast_to_ip function expected {[IP],[STRING]}, but got [INTEGER]"; assertEquals(expectedMsg, actualMsg); // From null or missing value From 24aa29ebc24d8da519a7ea0c8f1582f37f73f12a Mon Sep 17 00:00:00 2001 From: currantw Date: Mon, 2 Dec 2024 15:37:59 -0800 Subject: [PATCH 06/26] Assert that comparison only valid if same type, update tests accordingly Signed-off-by: currantw --- .../sql/data/model/ExprIpValue.java | 30 ++++++++----------- .../sql/data/model/ExprIpValueTest.java | 6 ++-- .../convert/TypeCastOperatorTest.java | 3 +- .../data/type/OpenSearchDataType.java | 2 ++ .../value/OpenSearchExprValueFactoryTest.java | 3 +- 5 files changed, 22 insertions(+), 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 index bbf4bd8c59c..6207796fcba 100644 --- 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 index e985937df5d..0e248cf05f8 100644 --- 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 index 80df91b6f52..c8c365b5d19 100644 --- 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 index 6c8912be864..2bcad9576a5 100644 --- 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 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/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java index 90efba3a64e..bbbdfee684a 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java +++ b/opensearch/src/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(); From 021b72770c0290aa61d85e78e7e642f4752200cc Mon Sep 17 00:00:00 2001 From: currantw Date: Mon, 2 Dec 2024 18:41:40 -0800 Subject: [PATCH 07/26] Add additional tests to increase code coverage Signed-off-by: currantw --- .../sql/data/model/ExprIpValue.java | 5 +-- .../sql/data/model/ExprIpValueTest.java | 33 +++++++++++++++++++ 2 files changed, 36 insertions(+), 2 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 index 6207796fcba..37d0d1a9556 100644 --- a/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java +++ b/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java @@ -34,7 +34,9 @@ public ExprIpValue(String s) { IPAddress address = new IPAddressString(s, validationOptions).toAddress(); // Convert IPv6 mapped IPv4 addresses to IPv4 - if (address.isIPv4Convertible()) address = address.toIPv4(); + if (address.isIPv4Convertible()) { + address = address.toIPv4(); + } value = address; } catch (AddressStringException e) { @@ -55,7 +57,6 @@ public ExprType type() { @Override public int compare(ExprValue other) { - assert (other instanceof ExprIpValue); // Map IPv4 addresses to IPv6 for comparison IPv6Address ipv6Value = toIPv6Address(value); 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 index 0e248cf05f8..8a2f5179207 100644 --- a/core/src/test/java/org/opensearch/sql/data/model/ExprIpValueTest.java +++ b/core/src/test/java/org/opensearch/sql/data/model/ExprIpValueTest.java @@ -6,12 +6,14 @@ package org.opensearch.sql.data.model; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.List; import org.junit.jupiter.api.Test; import org.opensearch.sql.data.type.ExprCoreType; +import org.opensearch.sql.exception.ExpressionEvaluationException; import org.opensearch.sql.exception.SemanticCheckException; public class ExprIpValueTest { @@ -74,6 +76,9 @@ public void testType() { @Test public void testCompare() { + Exception ex; + + // Compare to IP address. ipv4LesserStrings.forEach( (s) -> assertTrue(exprIpv4Value.compareTo(ExprValueUtils.ipValue(s)) > 0)); ipv4EqualStrings.forEach( @@ -86,12 +91,40 @@ public void testCompare() { (s) -> assertEquals(0, exprIpv6Value.compareTo(ExprValueUtils.ipValue(s)))); ipv6GreaterStrings.forEach( (s) -> assertTrue(exprIpv6Value.compareTo(ExprValueUtils.ipValue(s)) < 0)); + + // Compare to null/missing value. + ex = + assertThrows( + IllegalStateException.class, + () -> exprIpv4Value.compareTo(ExprValueUtils.LITERAL_NULL)); + assertEquals("[BUG] Unreachable, Comparing with NULL or MISSING is undefined", ex.getMessage()); + + ex = + assertThrows( + IllegalStateException.class, + () -> exprIpv4Value.compareTo(ExprValueUtils.LITERAL_MISSING)); + assertEquals("[BUG] Unreachable, Comparing with NULL or MISSING is undefined", ex.getMessage()); + + // Compare to other data type. + ex = + assertThrows( + ExpressionEvaluationException.class, + () -> exprIpv4Value.compareTo(ExprValueUtils.LITERAL_TRUE)); + assertEquals("compare expected value have same type, but with [IP, BOOLEAN]", ex.getMessage()); } @Test public void testEquals() { + assertEquals(exprIpv4Value, exprIpv4Value); + assertNotEquals(exprIpv4Value, new Object()); + assertNotEquals(exprIpv4Value, ExprValueUtils.LITERAL_NULL); + assertNotEquals(exprIpv4Value, ExprValueUtils.LITERAL_MISSING); + ipv4EqualStrings.forEach((s) -> assertEquals(exprIpv4Value, ExprValueUtils.ipValue(s))); ipv6EqualStrings.forEach((s) -> assertEquals(exprIpv6Value, ExprValueUtils.ipValue(s))); + + ipv4LesserStrings.forEach((s) -> assertNotEquals(exprIpv4Value, ExprValueUtils.ipValue(s))); + ipv6GreaterStrings.forEach((s) -> assertNotEquals(exprIpv6Value, ExprValueUtils.ipValue(s))); } @Test From d203425e73b082be677c84ab38192174fae30ff4 Mon Sep 17 00:00:00 2001 From: currantw Date: Wed, 4 Dec 2024 09:17:42 -0800 Subject: [PATCH 08/26] Integrate `cidrmatch` changes Signed-off-by: currantw --- .../sql/data/model/ExprIpValue.java | 46 ++------- .../opensearch/sql/data/model/ExprValue.java | 6 ++ .../sql/expression/ip/IPFunctions.java | 66 +++---------- .../org/opensearch/sql/utils/IPUtils.java | 97 +++++++++++++++++++ .../sql/data/model/ExprIpValueTest.java | 7 ++ .../sql/expression/ip/IPFunctionTest.java | 74 ++++++-------- .../org/opensearch/sql/legacy/JdbcTestIT.java | 4 +- .../org/opensearch/sql/ppl/IPFunctionIT.java | 15 ++- .../weblogs_index_mapping.json | 5 +- integ-test/src/test/resources/weblogs.json | 6 +- 10 files changed, 170 insertions(+), 156 deletions(-) create mode 100644 core/src/main/java/org/opensearch/sql/utils/IPUtils.java 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 index 37d0d1a9556..7ca2ffe92fb 100644 --- a/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java +++ b/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java @@ -5,44 +5,17 @@ package org.opensearch.sql.data.model; -import inet.ipaddr.AddressStringException; import inet.ipaddr.IPAddress; -import inet.ipaddr.IPAddressString; -import inet.ipaddr.IPAddressStringParameters; -import inet.ipaddr.ipv4.IPv4Address; -import inet.ipaddr.ipv6.IPv6Address; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; -import org.opensearch.sql.exception.SemanticCheckException; +import org.opensearch.sql.utils.IPUtils; /** Expression IP Address Value. */ public class ExprIpValue extends AbstractExprValue { private final IPAddress value; - private static final IPAddressStringParameters validationOptions = - new IPAddressStringParameters.Builder() - .allowEmpty(false) - .allowMask(false) - .allowPrefix(false) - .setEmptyAsLoopback(false) - .allow_inet_aton(false) - .allowSingleSegment(false) - .toParams(); - public ExprIpValue(String 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())); - } + value = IPUtils.toAddress(s); } @Override @@ -57,12 +30,7 @@ public ExprType type() { @Override public int compare(ExprValue other) { - - // Map IPv4 addresses to IPv6 for comparison - IPv6Address ipv6Value = toIPv6Address(value); - IPv6Address otherIpv6Value = toIPv6Address(((ExprIpValue) other).value); - - return ipv6Value.compareTo(otherIpv6Value); + return IPUtils.compare(value, ((ExprIpValue) other).value); } @Override @@ -75,10 +43,8 @@ public String toString() { return String.format("IP %s", value()); } - /** Returns the {@link IPv6Address} corresponding to the given {@link IPAddress}. */ - private static IPv6Address toIPv6Address(IPAddress ipAddress) { - return ipAddress instanceof IPv4Address iPv4Address - ? iPv4Address.toIPv6() - : (IPv6Address) ipAddress; + @Override + public IPAddress ipValue() { + return value; } } diff --git a/core/src/main/java/org/opensearch/sql/data/model/ExprValue.java b/core/src/main/java/org/opensearch/sql/data/model/ExprValue.java index 034ed22a753..da9c329f937 100644 --- a/core/src/main/java/org/opensearch/sql/data/model/ExprValue.java +++ b/core/src/main/java/org/opensearch/sql/data/model/ExprValue.java @@ -5,6 +5,7 @@ package org.opensearch.sql.data.model; +import inet.ipaddr.IPAddress; import java.io.Serializable; import java.time.Instant; import java.time.LocalDate; @@ -102,6 +103,11 @@ default Double doubleValue() { "invalid to get doubleValue from value of type " + type()); } + /** Get IP address value. */ + default IPAddress ipValue() { + throw new ExpressionEvaluationException("invalid to get ipValue from value of type " + type()); + } + /** Get string value. */ default String stringValue() { throw new ExpressionEvaluationException( diff --git a/core/src/main/java/org/opensearch/sql/expression/ip/IPFunctions.java b/core/src/main/java/org/opensearch/sql/expression/ip/IPFunctions.java index b3e7fad211d..e42c12841a1 100644 --- a/core/src/main/java/org/opensearch/sql/expression/ip/IPFunctions.java +++ b/core/src/main/java/org/opensearch/sql/expression/ip/IPFunctions.java @@ -6,14 +6,13 @@ package org.opensearch.sql.expression.ip; import static org.opensearch.sql.data.type.ExprCoreType.BOOLEAN; +import static org.opensearch.sql.data.type.ExprCoreType.IP; import static org.opensearch.sql.data.type.ExprCoreType.STRING; import static org.opensearch.sql.expression.function.FunctionDSL.define; import static org.opensearch.sql.expression.function.FunctionDSL.impl; import static org.opensearch.sql.expression.function.FunctionDSL.nullMissingHandling; -import inet.ipaddr.AddressStringException; -import inet.ipaddr.IPAddressString; -import inet.ipaddr.IPAddressStringParameters; +import inet.ipaddr.IPAddress; import lombok.experimental.UtilityClass; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.model.ExprValueUtils; @@ -21,6 +20,7 @@ import org.opensearch.sql.expression.function.BuiltinFunctionName; import org.opensearch.sql.expression.function.BuiltinFunctionRepository; import org.opensearch.sql.expression.function.DefaultFunctionResolver; +import org.opensearch.sql.utils.IPUtils; /** Utility class that defines and registers IP functions. */ @UtilityClass @@ -31,20 +31,17 @@ public void register(BuiltinFunctionRepository repository) { } private DefaultFunctionResolver cidrmatch() { - - // TODO #3145: Add support for IP address data type. return define( BuiltinFunctionName.CIDRMATCH.getName(), - impl(nullMissingHandling(IPFunctions::exprCidrMatch), BOOLEAN, STRING, STRING)); + impl(nullMissingHandling(IPFunctions::exprCidrMatch), BOOLEAN, IP, STRING)); } /** * Returns whether the given IP address is within the specified inclusive CIDR IP address range. * Supports both IPv4 and IPv6 addresses. * - * @param addressExprValue IP address as a string (e.g. "198.51.100.14" or - * "2001:0db8::ff00:42:8329"). - * @param rangeExprValue IP address range in CIDR notation as a string (e.g. "198.51.100.0/24" or + * @param addressExprValue IP address (e.g. "198.51.100.14" or "2001:0db8::ff00:42:8329"). + * @param rangeExprValue IP address range string in CIDR notation (e.g. "198.51.100.0/24" or * "2001:0db8::/32") * @return true if the address is in the range; otherwise false. * @throws SemanticCheckException if the address or range is not valid, or if they do not use the @@ -52,54 +49,17 @@ private DefaultFunctionResolver cidrmatch() { */ private ExprValue exprCidrMatch(ExprValue addressExprValue, ExprValue rangeExprValue) { - // TODO #3145: Update to support IP address data type. - String addressString = addressExprValue.stringValue(); - String rangeString = rangeExprValue.stringValue(); - - final IPAddressStringParameters validationOptions = - new IPAddressStringParameters.Builder() - .allowEmpty(false) - .setEmptyAsLoopback(false) - .allow_inet_aton(false) - .allowSingleSegment(false) - .toParams(); - - // Get and validate IP address. - IPAddressString address = - new IPAddressString(addressExprValue.stringValue(), validationOptions); - - try { - address.validate(); - } catch (AddressStringException e) { - String msg = - String.format( - "IP address '%s' is not valid. Error details: %s", addressString, e.getMessage()); - throw new SemanticCheckException(msg, e); - } - - // Get and validate CIDR IP address range. - IPAddressString range = new IPAddressString(rangeExprValue.stringValue(), validationOptions); + IPAddress address = addressExprValue.ipValue(); + IPAddress range = IPUtils.toRange(rangeExprValue.stringValue()); - try { - range.validate(); - } catch (AddressStringException e) { - String msg = - String.format( - "CIDR IP address range '%s' is not valid. Error details: %s", - rangeString, e.getMessage()); - throw new SemanticCheckException(msg, e); + if (IPUtils.compare(address, range.getLower()) < 0) { + return ExprValueUtils.LITERAL_FALSE; } - // Address and range must use the same IP version (IPv4 or IPv6). - if (address.isIPv4() ^ range.isIPv4()) { - String msg = - String.format( - "IP address '%s' and CIDR IP address range '%s' are not compatible. Both must be" - + " either IPv4 or IPv6.", - addressString, rangeString); - throw new SemanticCheckException(msg); + if (IPUtils.compare(address, range.getUpper()) > 0) { + return ExprValueUtils.LITERAL_FALSE; } - return ExprValueUtils.booleanValue(range.contains(address)); + return ExprValueUtils.LITERAL_TRUE; } } diff --git a/core/src/main/java/org/opensearch/sql/utils/IPUtils.java b/core/src/main/java/org/opensearch/sql/utils/IPUtils.java new file mode 100644 index 00000000000..44d010c085d --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/utils/IPUtils.java @@ -0,0 +1,97 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.utils; + +import inet.ipaddr.AddressStringException; +import inet.ipaddr.IPAddress; +import inet.ipaddr.IPAddressString; +import inet.ipaddr.IPAddressStringParameters; +import inet.ipaddr.ipv4.IPv4Address; +import inet.ipaddr.ipv6.IPv6Address; +import lombok.experimental.UtilityClass; +import org.opensearch.sql.exception.SemanticCheckException; + +@UtilityClass +public class IPUtils { + + // Parameters for IP address strings. + private static final IPAddressStringParameters.Builder commonValidationOptions = + new IPAddressStringParameters.Builder() + .allowEmpty(false) + .allowMask(false) + .setEmptyAsLoopback(false) + .allowPrefixOnly(false) + .allow_inet_aton(false) + .allowSingleSegment(false); + + private static final IPAddressStringParameters ipAddressStringParameters = + commonValidationOptions.allowPrefix(false).toParams(); + private static final IPAddressStringParameters ipAddressRangeStringParameters = + commonValidationOptions.allowPrefix(true).toParams(); + + /** + * Builds and returns the {@link IPAddress} represented by the given IP address range string in + * CIDR (classless inter-domain routing) notation. Returns {@link SemanticCheckException} if it + * does not represent a valid IP address range. Supports both IPv4 and IPv6 address ranges. + */ + public static IPAddress toRange(String s) throws SemanticCheckException { + try { + IPAddress range = new IPAddressString(s, ipAddressRangeStringParameters).toAddress(); + + // Convert IPv6 mapped address range to IPv4. + if (range.isIPv4Convertible()) { + final int prefixLength = range.getPrefixLength(); + range = range.toIPv4().setPrefixLength(prefixLength, false); + } + + return range; + + } catch (AddressStringException e) { + final String errorFormat = "IP address range string '%s' is not valid. Error details: %s"; + throw new SemanticCheckException(String.format(errorFormat, s, e.getMessage()), e); + } + } + + /** + * Builds and returns the {@link IPAddress} represented to the given IP address string. Throws + * {@link SemanticCheckException} if it does not represent a valid IP address. Supports both IPv4 + * and IPv6 addresses. + */ + public static IPAddress toAddress(String s) throws SemanticCheckException { + try { + IPAddress address = new IPAddressString(s, ipAddressStringParameters).toAddress(); + + // Convert IPv6 mapped address to IPv4. + if (address.isIPv4Convertible()) { + address = address.toIPv4(); + } + + return address; + } catch (AddressStringException e) { + final String errorFormat = "IP address string '%s' is not valid. Error details: %s"; + throw new SemanticCheckException(String.format(errorFormat, s, e.getMessage()), e); + } + } + + /** + * Compares the given {@link IPAddress} objects for order. Returns a negative integer, zero, or a + * positive integer if the first {@link IPAddress} object is less than, equal to, or greater than + * the second one. IPv4 addresses are mapped to IPv6 for comparison. + */ + public static int compare(IPAddress a, IPAddress b) { + final IPv6Address ipv6A = toIPv6Address(a); + final IPv6Address ipv6B = toIPv6Address(b); + + return ipv6A.compareTo(ipv6B); + } + + /** Returns the {@link IPv6Address} corresponding to the given {@link IPAddress}. */ + private static IPv6Address toIPv6Address(IPAddress ipAddress) { + return ipAddress instanceof IPv4Address iPv4Address + ? iPv4Address.toIPv6() + : (IPv6Address) ipAddress; + } +} 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 index 8a2f5179207..c414bafabf2 100644 --- a/core/src/test/java/org/opensearch/sql/data/model/ExprIpValueTest.java +++ b/core/src/test/java/org/opensearch/sql/data/model/ExprIpValueTest.java @@ -15,6 +15,7 @@ import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.exception.ExpressionEvaluationException; import org.opensearch.sql.exception.SemanticCheckException; +import org.opensearch.sql.utils.IPUtils; public class ExprIpValueTest { @@ -136,4 +137,10 @@ public void testToString() { (s) -> assertEquals(String.format("IP %s", ipv6String), ExprValueUtils.ipValue(s).toString())); } + + @Test + public void testIpValue() { + ipv4EqualStrings.forEach((s) -> assertEquals(IPUtils.toAddress(s), exprIpv4Value.ipValue())); + ipv6EqualStrings.forEach((s) -> assertEquals(IPUtils.toAddress(s), exprIpv6Value.ipValue())); + } } diff --git a/core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionTest.java b/core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionTest.java index b50bf9fd1f9..1991da0ad70 100644 --- a/core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionTest.java @@ -11,7 +11,7 @@ import static org.mockito.Mockito.when; import static org.opensearch.sql.data.model.ExprValueUtils.LITERAL_FALSE; import static org.opensearch.sql.data.model.ExprValueUtils.LITERAL_TRUE; -import static org.opensearch.sql.data.type.ExprCoreType.STRING; +import static org.opensearch.sql.data.type.ExprCoreType.IP; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -30,90 +30,72 @@ public class IPFunctionTest { // IP range and address constants for testing. private static final ExprValue IPv4Range = ExprValueUtils.stringValue("198.51.100.0/24"); + private static final ExprValue IPv4RangeMapped = + ExprValueUtils.stringValue("::ffff:198.51.100.0/24"); private static final ExprValue IPv6Range = ExprValueUtils.stringValue("2001:0db8::/32"); - // TODO #3145: Add tests for IP address data type. - private static final ExprValue IPv4AddressBelow = ExprValueUtils.stringValue("198.51.99.1"); - private static final ExprValue IPv4AddressWithin = ExprValueUtils.stringValue("198.51.100.1"); - private static final ExprValue IPv4AddressAbove = ExprValueUtils.stringValue("198.51.101.2"); + private static final ExprValue IPv4AddressBelow = ExprValueUtils.ipValue("198.51.99.1"); + private static final ExprValue IPv4AddressWithin = ExprValueUtils.ipValue("198.51.100.1"); + private static final ExprValue IPv4AddressAbove = ExprValueUtils.ipValue("198.51.101.2"); private static final ExprValue IPv6AddressBelow = - ExprValueUtils.stringValue("2001:0db7::ff00:42:8329"); + ExprValueUtils.ipValue("2001:0db7::ff00:42:8329"); private static final ExprValue IPv6AddressWithin = - ExprValueUtils.stringValue("2001:0db8::ff00:42:8329"); + ExprValueUtils.ipValue("2001:0db8::ff00:42:8329"); private static final ExprValue IPv6AddressAbove = - ExprValueUtils.stringValue("2001:0db9::ff00:42:8329"); + ExprValueUtils.ipValue("2001:0db9::ff00:42:8329"); // Mock value environment for testing. @Mock private Environment env; @Test - public void cidrmatch_invalid_address() { - SemanticCheckException exception = + public void cidrmatch_invalid_arguments() { + Exception ex; + + ex = assertThrows( SemanticCheckException.class, - () -> execute(ExprValueUtils.stringValue("INVALID"), IPv4Range)); + () -> execute(ExprValueUtils.ipValue("INVALID"), IPv4Range)); assertTrue( - exception.getMessage().matches("IP address 'INVALID' is not valid. Error details: .*")); - } + ex.getMessage().matches("IP address string 'INVALID' is not valid. Error details: .*")); - @Test - public void cidrmatch_invalid_range() { - SemanticCheckException exception = + ex = assertThrows( SemanticCheckException.class, () -> execute(IPv4AddressWithin, ExprValueUtils.stringValue("INVALID"))); assertTrue( - exception - .getMessage() - .matches("CIDR IP address range 'INVALID' is not valid. Error details: .*")); + ex.getMessage() + .matches("IP address range string 'INVALID' is not valid. Error details: .*")); } @Test - public void cidrmatch_different_versions() { - SemanticCheckException exception; - - exception = - assertThrows(SemanticCheckException.class, () -> execute(IPv4AddressWithin, IPv6Range)); - assertEquals( - "IP address '198.51.100.1' and CIDR IP address range '2001:0db8::/32' are not compatible." - + " Both must be either IPv4 or IPv6.", - exception.getMessage()); - - exception = - assertThrows(SemanticCheckException.class, () -> execute(IPv6AddressWithin, IPv4Range)); - assertEquals( - "IP address '2001:0db8::ff00:42:8329' and CIDR IP address range '198.51.100.0/24' are not" - + " compatible. Both must be either IPv4 or IPv6.", - exception.getMessage()); - } + public void cidrmatch_valid_arguments() { - @Test - public void cidrmatch_valid_ipv4() { assertEquals(LITERAL_FALSE, execute(IPv4AddressBelow, IPv4Range)); assertEquals(LITERAL_TRUE, execute(IPv4AddressWithin, IPv4Range)); assertEquals(LITERAL_FALSE, execute(IPv4AddressAbove, IPv4Range)); - } - @Test - public void cidrmatch_valid_ipv6() { + assertEquals(LITERAL_FALSE, execute(IPv4AddressBelow, IPv4RangeMapped)); + assertEquals(LITERAL_TRUE, execute(IPv4AddressWithin, IPv4RangeMapped)); + assertEquals(LITERAL_FALSE, execute(IPv4AddressAbove, IPv4RangeMapped)); + assertEquals(LITERAL_FALSE, execute(IPv6AddressBelow, IPv6Range)); assertEquals(LITERAL_TRUE, execute(IPv6AddressWithin, IPv6Range)); assertEquals(LITERAL_FALSE, execute(IPv6AddressAbove, IPv6Range)); } /** - * Builds and evaluates a CIDR function expression with the given field and range expression - * values, and returns the resulting value. + * Builds and evaluates a {@code cidrmatch} function expression with the given address and range + * expression values, and returns the resulting value. */ - private ExprValue execute(ExprValue field, ExprValue range) { + private ExprValue execute(ExprValue address, ExprValue range) { final String fieldName = "ip_address"; - FunctionExpression exp = DSL.cidrmatch(DSL.ref(fieldName, STRING), DSL.literal(range)); + FunctionExpression exp = DSL.cidrmatch(DSL.ref(fieldName, IP), DSL.literal(range)); // Mock the value environment to return the specified field // expression as the value for the "ip_address" field. - when(DSL.ref(fieldName, STRING).valueOf(env)).thenReturn(field); + when(DSL.ref(fieldName, IP).valueOf(env)).thenReturn(address); return exp.valueOf(env); } diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/JdbcTestIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/JdbcTestIT.java index 005119a9bce..68d43288388 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/JdbcTestIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/JdbcTestIT.java @@ -155,9 +155,9 @@ public void dateFunctionNameCaseInsensitiveTest() { public void ipTypeShouldPassJdbcFormatter() { assertThat( executeQuery( - "SELECT host_ip AS hostIP FROM " + "SELECT host FROM " + TestsConstants.TEST_INDEX_WEBLOG - + " ORDER BY hostIP", + + " ORDER BY host", "jdbc"), containsString("\"type\": \"ip\"")); } diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java index adb044d0d2e..e85773fdb49 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java @@ -24,35 +24,34 @@ public void init() throws IOException { @Test public void test_cidrmatch() throws IOException { - - // TODO #3145: Add tests for IP address data type. + JSONObject result; // No matches result = executeQuery( String.format( - "source=%s | where cidrmatch(host_string, '199.120.111.0/24') | fields host_string", + "source=%s | where cidrmatch(host, '199.120.111.0/24') | fields host", TEST_INDEX_WEBLOG)); - verifySchema(result, schema("host_string", null, "string")); + verifySchema(result, schema("host", null, "ip")); verifyDataRows(result); // One match result = executeQuery( String.format( - "source=%s | where cidrmatch(host_string, '199.120.110.0/24') | fields host_string", + "source=%s | where cidrmatch(host, '199.120.110.0/24') | fields host", TEST_INDEX_WEBLOG)); - verifySchema(result, schema("host_string", null, "string")); + verifySchema(result, schema("host", null, "ip")); verifyDataRows(result, rows("199.120.110.21")); // Multiple matches result = executeQuery( String.format( - "source=%s | where cidrmatch(host_string, '199.0.0.0/8') | fields host_string", + "source=%s | where cidrmatch(host, '199.0.0.0/8') | fields host", TEST_INDEX_WEBLOG)); - verifySchema(result, schema("host_string", null, "string")); + verifySchema(result, schema("host", null, "ip")); verifyDataRows(result, rows("199.72.81.55"), rows("199.120.110.21")); } } diff --git a/integ-test/src/test/resources/indexDefinitions/weblogs_index_mapping.json b/integ-test/src/test/resources/indexDefinitions/weblogs_index_mapping.json index bff3e20bb94..05b9784313a 100644 --- a/integ-test/src/test/resources/indexDefinitions/weblogs_index_mapping.json +++ b/integ-test/src/test/resources/indexDefinitions/weblogs_index_mapping.json @@ -1,12 +1,9 @@ { "mappings": { "properties": { - "host_ip": { + "host": { "type": "ip" }, - "host_string": { - "type": "keyword" - }, "method": { "type": "text" }, diff --git a/integ-test/src/test/resources/weblogs.json b/integ-test/src/test/resources/weblogs.json index d2e9a968f84..4228e9c4d2a 100644 --- a/integ-test/src/test/resources/weblogs.json +++ b/integ-test/src/test/resources/weblogs.json @@ -1,6 +1,6 @@ {"index":{}} -{"host_ip": "199.72.81.55", "host_string": "199.72.81.55", "method": "GET", "url": "/history/apollo/", "response": "200", "bytes": "6245"} +{"host": "199.72.81.55", "method": "GET", "url": "/history/apollo/", "response": "200", "bytes": "6245"} {"index":{}} -{"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"} +{"host": "199.120.110.21", "method": "GET", "url": "/shuttle/missions/sts-73/mission-sts-73.html", "response": "200", "bytes": "4085"} {"index":{}} -{"host_ip": "205.212.115.106", "host_string": "205.212.115.106", "method": "GET", "url": "/shuttle/countdown/countdown.html", "response": "200", "bytes": "3985"} +{"host": "205.212.115.106", "method": "GET", "url": "/shuttle/countdown/countdown.html", "response": "200", "bytes": "3985"} From aadc8f8ccdef8ec28ec56b2f9b51f8dab8c50710 Mon Sep 17 00:00:00 2001 From: currantw Date: Wed, 4 Dec 2024 10:56:16 -0800 Subject: [PATCH 09/26] Remove `OpenSearchIPType` data type Signed-off-by: currantw --- .../org/opensearch/sql/legacy/JdbcTestIT.java | 5 +-- .../org/opensearch/sql/ppl/IPFunctionIT.java | 2 +- .../data/type/OpenSearchDataType.java | 2 -- .../data/type/OpenSearchIpType.java | 34 ------------------- .../value/OpenSearchExprValueFactory.java | 20 +++++------ .../value/OpenSearchExprValueFactoryTest.java | 9 +++-- 6 files changed, 15 insertions(+), 57 deletions(-) delete mode 100644 opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchIpType.java diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/JdbcTestIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/JdbcTestIT.java index 68d43288388..b937eea5b32 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/JdbcTestIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/JdbcTestIT.java @@ -155,10 +155,7 @@ public void dateFunctionNameCaseInsensitiveTest() { public void ipTypeShouldPassJdbcFormatter() { assertThat( executeQuery( - "SELECT host FROM " - + TestsConstants.TEST_INDEX_WEBLOG - + " ORDER BY host", - "jdbc"), + "SELECT host FROM " + TestsConstants.TEST_INDEX_WEBLOG + " ORDER BY host", "jdbc"), containsString("\"type\": \"ip\"")); } diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java index e85773fdb49..c93296192ea 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java @@ -24,7 +24,7 @@ public void init() throws IOException { @Test public void test_cidrmatch() throws IOException { - + JSONObject result; // No matches 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 index 2bcad9576a5..6c8912be864 100644 --- 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,8 +160,6 @@ public static OpenSearchDataType of(MappingType mappingType, Map 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/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 deleted file mode 100644 index e7e90ae815d..00000000000 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchIpType.java +++ /dev/null @@ -1,34 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.opensearch.data.type; - -import static org.opensearch.sql.data.type.ExprCoreType.IP; - -import lombok.EqualsAndHashCode; - -/** - * The type of an ip value. See doc - */ -@EqualsAndHashCode(callSuper = false) -public class OpenSearchIpType extends OpenSearchDataType { - - private static final OpenSearchIpType instance = new OpenSearchIpType(); - - private OpenSearchIpType() { - super(MappingType.Ip); - exprCoreType = IP; - } - - public static OpenSearchIpType of() { - return OpenSearchIpType.instance; - } - - @Override - protected OpenSearchDataType cloneEmpty() { - return instance; - } -} diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java index 401f270f075..68c6fda617f 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java @@ -11,6 +11,7 @@ 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.IP; import static org.opensearch.sql.data.type.ExprCoreType.LONG; import static org.opensearch.sql.data.type.ExprCoreType.STRING; import static org.opensearch.sql.data.type.ExprCoreType.STRUCT; @@ -64,7 +65,6 @@ import org.opensearch.sql.opensearch.data.type.OpenSearchBinaryType; 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.utils.Content; import org.opensearch.sql.opensearch.data.utils.ObjectContent; import org.opensearch.sql.opensearch.data.utils.OpenSearchJsonContent; @@ -203,14 +203,12 @@ private ExprValue parse( } 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 { - 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())); - } + throw new IllegalStateException( + String.format( + "Unsupported type: %s for value: %s.", type.typeName(), content.objectValue())); } } @@ -419,10 +417,10 @@ private ExprValue parseGeoPoint(Content content, boolean supportArrays) { */ private ExprValue parseInnerArrayValue( Content content, String prefix, ExprType type, boolean supportArrays) { - if (type instanceof OpenSearchIpType - || type instanceof OpenSearchBinaryType - || type instanceof OpenSearchDateType) { + if (type instanceof OpenSearchBinaryType || type instanceof OpenSearchDateType) { return parse(content, prefix, Optional.of(type), supportArrays); + } else if (content.isString() && type.equals(OpenSearchDataType.of(IP))) { + return parse(content, prefix, Optional.of(OpenSearchDataType.of(IP)), supportArrays); } else if (content.isString()) { return parse(content, prefix, Optional.of(OpenSearchDataType.of(STRING)), supportArrays); } else if (content.isLong()) { diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java index bbbdfee684a..ad2f5988b91 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java @@ -59,7 +59,6 @@ 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; @@ -117,7 +116,7 @@ class OpenSearchExprValueFactoryTest { "textKeywordV", OpenSearchTextType.of( Map.of("words", OpenSearchDataType.of(OpenSearchDataType.MappingType.Keyword)))) - .put(fieldIp, OpenSearchIpType.of(OpenSearchDataType.MappingType.Ip)) + .put(fieldIp, OpenSearchDataType.of(OpenSearchDataType.MappingType.Ip)) .put("geoV", OpenSearchDataType.of(OpenSearchDataType.MappingType.GeoPoint)) .put("binaryV", OpenSearchDataType.of(OpenSearchDataType.MappingType.Binary)) .build(); @@ -688,10 +687,10 @@ public void constructBinaryArrayReturnsAll() { public void constructArrayOfIPsReturnsAll() { final String ipv4String = "1.2.3.4"; final String ipv6String = "2001:db7::ff00:42:8329"; + assertEquals( - new ExprCollectionValue(List.of(ipValue(ipv4String), ipValue(ipv6String))), - tupleValue(String.format("{\"%s\":[\"%s\",\"%s\"]}", fieldIp, ipv4String, ipv6String)) - .get(fieldIp)); + new ExprCollectionValue(List.of(ipValue("1.2.3.4"), ipValue("2001:db7::ff00:42:8329"))), + tupleValue("{\"ipV\":[" + "\"1.2.3.4\"," + "\"2001:db7::ff00:42:8329\"" + "]}").get("ipV")); } @Test From 1aed13ea75a8982a4eb9d6b40537299a007220da Mon Sep 17 00:00:00 2001 From: currantw Date: Wed, 4 Dec 2024 15:43:19 -0800 Subject: [PATCH 10/26] Fix more failing tests Signed-off-by: currantw --- .../sql/data/model/ExprValueUtils.java | 5 +++ .../sql/data/model/ExprIpValueTest.java | 40 ++++++++----------- .../sql/data/model/ExprValueUtilsTest.java | 7 +++- .../opensearch/sql/utils/ComparisonUtil.java | 2 + 4 files changed, 29 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java b/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java index ed9f0b98efe..890e0ef8d51 100644 --- a/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java +++ b/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java @@ -5,6 +5,7 @@ package org.opensearch.sql.data.model; +import inet.ipaddr.IPAddress; import java.time.Instant; import java.time.LocalDate; import java.time.LocalDateTime; @@ -192,6 +193,10 @@ public static Map getTupleValue(ExprValue exprValue) { return exprValue.tupleValue(); } + public static IPAddress getIpValue(ExprValue exprValue) { + return exprValue.ipValue(); + } + public static Boolean getBooleanValue(ExprValue exprValue) { return exprValue.booleanValue(); } 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 index c414bafabf2..b0ef598a5ae 100644 --- a/core/src/test/java/org/opensearch/sql/data/model/ExprIpValueTest.java +++ b/core/src/test/java/org/opensearch/sql/data/model/ExprIpValueTest.java @@ -54,13 +54,10 @@ public class ExprIpValueTest { @Test public void testInvalid() { - Exception ex = - assertThrows(SemanticCheckException.class, () -> ExprValueUtils.ipValue(ipInvalidString)); - assertTrue( - ex.getMessage() - .matches( - String.format( - "IP address string '%s' is not valid. Error details: .*", ipInvalidString))); + assertThrows( + SemanticCheckException.class, + () -> ExprValueUtils.ipValue(ipInvalidString), + String.format("IP address string '%s' is not valid. Error details: .*", ipInvalidString)); } @Test @@ -77,7 +74,6 @@ public void testType() { @Test public void testCompare() { - Exception ex; // Compare to IP address. ipv4LesserStrings.forEach( @@ -94,24 +90,20 @@ public void testCompare() { (s) -> assertTrue(exprIpv6Value.compareTo(ExprValueUtils.ipValue(s)) < 0)); // Compare to null/missing value. - ex = - assertThrows( - IllegalStateException.class, - () -> exprIpv4Value.compareTo(ExprValueUtils.LITERAL_NULL)); - assertEquals("[BUG] Unreachable, Comparing with NULL or MISSING is undefined", ex.getMessage()); - - ex = - assertThrows( - IllegalStateException.class, - () -> exprIpv4Value.compareTo(ExprValueUtils.LITERAL_MISSING)); - assertEquals("[BUG] Unreachable, Comparing with NULL or MISSING is undefined", ex.getMessage()); + assertThrows( + IllegalStateException.class, + () -> exprIpv4Value.compareTo(ExprValueUtils.LITERAL_NULL), + "[BUG] Unreachable, Comparing with NULL or MISSING is undefined"); + assertThrows( + IllegalStateException.class, + () -> exprIpv4Value.compareTo(ExprValueUtils.LITERAL_MISSING), + "[BUG] Unreachable, Comparing with NULL or MISSING is undefined"); // Compare to other data type. - ex = - assertThrows( - ExpressionEvaluationException.class, - () -> exprIpv4Value.compareTo(ExprValueUtils.LITERAL_TRUE)); - assertEquals("compare expected value have same type, but with [IP, BOOLEAN]", ex.getMessage()); + assertThrows( + ExpressionEvaluationException.class, + () -> exprIpv4Value.compareTo(ExprValueUtils.LITERAL_TRUE), + "compare expected value have same type, but with [IP, BOOLEAN]"); } @Test diff --git a/core/src/test/java/org/opensearch/sql/data/model/ExprValueUtilsTest.java b/core/src/test/java/org/opensearch/sql/data/model/ExprValueUtilsTest.java index 9fe63471028..48db530a947 100644 --- a/core/src/test/java/org/opensearch/sql/data/model/ExprValueUtilsTest.java +++ b/core/src/test/java/org/opensearch/sql/data/model/ExprValueUtilsTest.java @@ -14,6 +14,7 @@ import static org.opensearch.sql.data.type.ExprCoreType.BOOLEAN; import static org.opensearch.sql.data.type.ExprCoreType.DATE; import static org.opensearch.sql.data.type.ExprCoreType.INTERVAL; +import static org.opensearch.sql.data.type.ExprCoreType.IP; import static org.opensearch.sql.data.type.ExprCoreType.STRING; import static org.opensearch.sql.data.type.ExprCoreType.STRUCT; import static org.opensearch.sql.data.type.ExprCoreType.TIME; @@ -47,6 +48,7 @@ import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.exception.ExpressionEvaluationException; import org.opensearch.sql.storage.bindingtuple.BindingTuple; +import org.opensearch.sql.utils.IPUtils; @DisplayName("Test Expression Value Utils") public class ExprValueUtilsTest { @@ -63,6 +65,7 @@ public class ExprValueUtilsTest { private static final List nonNumberValues = Arrays.asList( + new ExprIpValue("1.2.3.4"), new ExprStringValue("1"), ExprBooleanValue.of(true), new ExprCollectionValue(ImmutableList.of(new ExprIntegerValue(1))), @@ -85,6 +88,7 @@ public class ExprValueUtilsTest { ExprValueUtils::getDoubleValue); private static final List> nonNumberValueExtractor = Arrays.asList( + ExprValueUtils::getIpValue, ExprValueUtils::getStringValue, ExprValueUtils::getBooleanValue, ExprValueUtils::getCollectionValue, @@ -109,7 +113,7 @@ public class ExprValueUtilsTest { ExprCoreType.FLOAT, ExprCoreType.DOUBLE); private static final List nonNumberTypes = - Arrays.asList(STRING, BOOLEAN, ARRAY, STRUCT); + Arrays.asList(IP, STRING, BOOLEAN, ARRAY, STRUCT); private static final List dateAndTimeTypes = Arrays.asList(DATE, TIME, TIMESTAMP, INTERVAL); private static final List allTypes = @@ -124,6 +128,7 @@ private static Stream getValueTestArgumentStream() { 1L, 1f, 1D, + IPUtils.toAddress("1.2.3.4"), "1", true, Arrays.asList(integerValue(1)), diff --git a/core/src/test/java/org/opensearch/sql/utils/ComparisonUtil.java b/core/src/test/java/org/opensearch/sql/utils/ComparisonUtil.java index 0d9fe80339a..ebc60c59b95 100644 --- a/core/src/test/java/org/opensearch/sql/utils/ComparisonUtil.java +++ b/core/src/test/java/org/opensearch/sql/utils/ComparisonUtil.java @@ -69,6 +69,8 @@ public static int compare(ExprValue v1, ExprValue v2) { return v1.dateValue().compareTo(v2.dateValue()); case TIMESTAMP: return v1.timestampValue().compareTo(v2.timestampValue()); + case IP: + return v1.ipValue().compareTo(v2.ipValue()); default: throw new ExpressionEvaluationException( String.format("%s instances are not comparable", v1.getClass().getSimpleName())); From 47746e44b41c2a2b3f86bfc0737783cff99ef425 Mon Sep 17 00:00:00 2001 From: currantw Date: Thu, 5 Dec 2024 10:45:32 -0800 Subject: [PATCH 11/26] Minor cleanup Signed-off-by: currantw --- .../org/opensearch/sql/utils/IPUtils.java | 2 +- .../sql/expression/ip/IPFunctionTest.java | 25 ++++++------------- .../convert/TypeCastOperatorTest.java | 20 ++++++--------- .../opensearch/sql/utils/ComparisonUtil.java | 2 -- .../script/filter/FilterQueryBuilderTest.java | 19 +++++++------- 5 files changed, 26 insertions(+), 42 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/utils/IPUtils.java b/core/src/main/java/org/opensearch/sql/utils/IPUtils.java index 44d010c085d..8874823a030 100644 --- a/core/src/main/java/org/opensearch/sql/utils/IPUtils.java +++ b/core/src/main/java/org/opensearch/sql/utils/IPUtils.java @@ -34,7 +34,7 @@ public class IPUtils { /** * Builds and returns the {@link IPAddress} represented by the given IP address range string in - * CIDR (classless inter-domain routing) notation. Returns {@link SemanticCheckException} if it + * CIDR (classless inter-domain routing) notation. Throws {@link SemanticCheckException} if it * does not represent a valid IP address range. Supports both IPv4 and IPv6 address ranges. */ public static IPAddress toRange(String s) throws SemanticCheckException { diff --git a/core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionTest.java b/core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionTest.java index 1991da0ad70..8cb5ca33aeb 100644 --- a/core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionTest.java @@ -7,7 +7,6 @@ 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.Mockito.when; import static org.opensearch.sql.data.model.ExprValueUtils.LITERAL_FALSE; import static org.opensearch.sql.data.model.ExprValueUtils.LITERAL_TRUE; @@ -50,22 +49,14 @@ public class IPFunctionTest { @Test public void cidrmatch_invalid_arguments() { - Exception ex; - - ex = - assertThrows( - SemanticCheckException.class, - () -> execute(ExprValueUtils.ipValue("INVALID"), IPv4Range)); - assertTrue( - ex.getMessage().matches("IP address string 'INVALID' is not valid. Error details: .*")); - - ex = - assertThrows( - SemanticCheckException.class, - () -> execute(IPv4AddressWithin, ExprValueUtils.stringValue("INVALID"))); - assertTrue( - ex.getMessage() - .matches("IP address range string 'INVALID' is not valid. Error details: .*")); + assertThrows( + SemanticCheckException.class, + () -> execute(ExprValueUtils.ipValue("INVALID"), IPv4Range), + "IP address string 'INVALID' is not valid. Error details: .*"); + assertThrows( + SemanticCheckException.class, + () -> execute(IPv4AddressWithin, ExprValueUtils.stringValue("INVALID")), + "IP address range string 'INVALID' is not valid. Error details: .*"); } @Test 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 index c8c365b5d19..fd579dfb470 100644 --- 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 @@ -343,9 +343,6 @@ void castToTimestamp() { void castToIp() { FunctionExpression exp; - String expectedMsg; - String actualMsg; - final String ipv4String = "1.2.3.4"; final String ipv6String = "2001:db7::ff00:42:8329"; final String ipInvalidString = "INVALID"; @@ -363,10 +360,10 @@ void castToIp() { assertEquals(exprIpv6Value, exp.valueOf()); exp = DSL.castIp(DSL.literal(ipInvalidString)); - actualMsg = assertThrows(SemanticCheckException.class, exp::valueOf).getMessage(); - expectedMsg = - String.format("IP address string '%s' is not valid. Error details: .*", ipInvalidString); - assertTrue(actualMsg.matches(expectedMsg)); + assertThrows( + SemanticCheckException.class, + exp::valueOf, + String.format("IP address string '%s' is not valid. Error details: .*", ipInvalidString)); // From IP address exp = DSL.castIp(DSL.literal(exprIpv4Value)); @@ -378,11 +375,10 @@ void castToIp() { assertEquals(exprIpv6Value, exp.valueOf()); // From invalid type - actualMsg = - assertThrows(ExpressionEvaluationException.class, () -> DSL.castIp(DSL.literal(0))) - .getMessage(); - expectedMsg = "cast_to_ip function expected {[IP],[STRING]}, but got [INTEGER]"; - assertEquals(expectedMsg, actualMsg); + assertThrows( + ExpressionEvaluationException.class, + () -> DSL.castIp(DSL.literal(0)), + "cast_to_ip function expected {[IP],[STRING]}, but got [INTEGER]"); // From null or missing value exp = DSL.castIp(DSL.literal(ExprNullValue.of())); diff --git a/core/src/test/java/org/opensearch/sql/utils/ComparisonUtil.java b/core/src/test/java/org/opensearch/sql/utils/ComparisonUtil.java index ebc60c59b95..0d9fe80339a 100644 --- a/core/src/test/java/org/opensearch/sql/utils/ComparisonUtil.java +++ b/core/src/test/java/org/opensearch/sql/utils/ComparisonUtil.java @@ -69,8 +69,6 @@ public static int compare(ExprValue v1, ExprValue v2) { return v1.dateValue().compareTo(v2.dateValue()); case TIMESTAMP: return v1.timestampValue().compareTo(v2.timestampValue()); - case IP: - return v1.ipValue().compareTo(v2.ipValue()); default: throw new ExpressionEvaluationException( String.format("%s instances are not comparable", v1.getClass().getSimpleName())); diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java index 832c92826a1..f8c43743abf 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/FilterQueryBuilderTest.java @@ -1907,16 +1907,15 @@ void non_literal_in_cast_should_build_script() { void non_cast_nested_function_should_build_script() { mockToStringSerializer(); assertJsonEquals( - """ - { - "script" : { - "script" : { - "source" : "=(integer_value, abs(+(1, 0)))", - "lang" : "opensearch_query_expression" - }, - "boost" : 1.0 - } - }""", + "{\n" + + " \"script\" : {\n" + + " \"script\" : {\n" + + " \"source\" : \"=(integer_value, abs(+(1, 0)))\",\n" + + " \"lang\" : \"opensearch_query_expression\"\n" + + " },\n" + + " \"boost\" : 1.0\n" + + " }\n" + + "}", buildQuery( DSL.equal(ref("integer_value", INTEGER), DSL.abs(DSL.add(literal(1), literal(0)))))); } From d03bd4f86835ce790a3233ddda6dd540b4dd5d03 Mon Sep 17 00:00:00 2001 From: currantw Date: Tue, 10 Dec 2024 18:03:50 -0800 Subject: [PATCH 12/26] Add new tests for IP data type to `SortCommandIT`, and update `weblogs` test data. Signed-off-by: currantw --- docs/user/ppl/functions/ip.rst | 15 ++++++++------- doctest/test_data/weblogs.json | 12 +++++++++--- .../org/opensearch/sql/ppl/IPFunctionIT.java | 11 +++++------ .../org/opensearch/sql/ppl/SortCommandIT.java | 16 ++++++++++++++++ integ-test/src/test/resources/weblogs.json | 12 +++++++++--- 5 files changed, 47 insertions(+), 19 deletions(-) diff --git a/docs/user/ppl/functions/ip.rst b/docs/user/ppl/functions/ip.rst index 3387974af58..eba1afa4153 100644 --- a/docs/user/ppl/functions/ip.rst +++ b/docs/user/ppl/functions/ip.rst @@ -22,13 +22,14 @@ Return type: BOOLEAN Example: - os> source=weblogs | where cidrmatch(host, '199.120.110.0/24') | fields host - fetched rows / total rows = 1/1 - +----------------+ - | host | - |----------------| - | 199.120.110.21 | - +----------------+ + os> source=weblogs | where cidrmatch(host, '1.0.0.0/24') | fields host, url + fetched rows / total rows = 2/2 + +----------------+--------------------| + | host | url | + +----------------+--------------------+ + | 1.2.3.4 | /history/voyager1/ | + | 1.2.3.5 | /history/voyager2/ | + +----------------+--------------------| Note: - `ip` can be an IPv4 or an IPv6 address diff --git a/doctest/test_data/weblogs.json b/doctest/test_data/weblogs.json index 4228e9c4d2a..67e322e0392 100644 --- a/doctest/test_data/weblogs.json +++ b/doctest/test_data/weblogs.json @@ -1,6 +1,12 @@ {"index":{}} -{"host": "199.72.81.55", "method": "GET", "url": "/history/apollo/", "response": "200", "bytes": "6245"} +{"host": "::1", "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": "0.0.0.2", "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": "::3", "method": "GET", "url": "/shuttle/countdown/countdown.html", "response": "200", "bytes": "3985"} +{"index":{}} +{"host": "::FFFF:1.2.3.4", "method": "GET", "url": "/history/voyager1/", "response": "200", "bytes": "1234"} +{"index":{}} +{"host": "1.2.3.4", "method": "GET", "url": "/history/voyager2/", "response": "200", "bytes": "4321"} +{"index":{}} +{"host": "::FFFF:1234", "method": "GET", "url": "/history/artemis/", "response": "200", "bytes": "9876"} \ No newline at end of file diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java index c93296192ea..b5994ed3814 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java @@ -31,7 +31,7 @@ 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, '250.0.0.0/24') | fields host", TEST_INDEX_WEBLOG)); verifySchema(result, schema("host", null, "ip")); verifyDataRows(result); @@ -40,18 +40,17 @@ public void test_cidrmatch() throws IOException { result = executeQuery( String.format( - "source=%s | where cidrmatch(host, '199.120.110.0/24') | fields host", + "source=%s | where cidrmatch(host, '0.0.0.0/24') | fields host", TEST_INDEX_WEBLOG)); verifySchema(result, schema("host", null, "ip")); - verifyDataRows(result, rows("199.120.110.21")); + verifyDataRows(result, rows("0.0.0.2")); // Multiple matches result = executeQuery( String.format( - "source=%s | where cidrmatch(host, '199.0.0.0/8') | fields host", - TEST_INDEX_WEBLOG)); + "source=%s | where cidrmatch(host, '1.2.3.0/8') | fields host", TEST_INDEX_WEBLOG)); verifySchema(result, schema("host", null, "ip")); - verifyDataRows(result, rows("199.72.81.55"), rows("199.120.110.21")); + verifyDataRows(result, rows("1.2.3.4"), rows("1.2.3.5")); } } diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java index 1061f0bd9d3..d3dae856c76 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java @@ -8,6 +8,7 @@ import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK; import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK_WITH_NULL_VALUES; import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DOG; +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_WEBLOG; import static org.opensearch.sql.util.MatcherUtils.rows; import static org.opensearch.sql.util.MatcherUtils.verifyOrder; @@ -28,6 +29,7 @@ public void init() throws IOException { loadIndex(Index.BANK); loadIndex(Index.BANK_WITH_NULL_VALUES); loadIndex(Index.DOG); + loadIndex(Index.WEBLOG); } @Test @@ -130,6 +132,20 @@ public void testSortStringField() throws IOException { rows("Ratliff")); } + @Test + public void testSortIpField() throws IOException { + final JSONObject result = + executeQuery(String.format("source=%s | sort host | fields host", TEST_INDEX_WEBLOG)); + verifyOrder( + result, + rows("::1"), + rows("::3"), + rows("::ffff:1234"), + rows("0.0.0.2"), + rows("1.2.3.4"), + rows("1.2.3.5")); + } + @Test public void testSortMultipleFields() throws IOException { JSONObject result = diff --git a/integ-test/src/test/resources/weblogs.json b/integ-test/src/test/resources/weblogs.json index 4228e9c4d2a..27d39b83be4 100644 --- a/integ-test/src/test/resources/weblogs.json +++ b/integ-test/src/test/resources/weblogs.json @@ -1,6 +1,12 @@ {"index":{}} -{"host": "199.72.81.55", "method": "GET", "url": "/history/apollo/", "response": "200", "bytes": "6245"} +{"host": "::1", "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": "0.0.0.2", "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": "::3", "method": "GET", "url": "/shuttle/countdown/countdown.html", "response": "200", "bytes": "3985"} +{"index":{}} +{"host": "::FFFF:1.2.3.4", "method": "GET", "url": "/history/voyager1/", "response": "200", "bytes": "1234"} +{"index":{}} +{"host": "1.2.3.5", "method": "GET", "url": "/history/voyager2/", "response": "200", "bytes": "4321"} +{"index":{}} +{"host": "::FFFF:1234", "method": "GET", "url": "/history/artemis/", "response": "200", "bytes": "9876"} From 7569d08a469568d37859b059b04e743beefeaf8f Mon Sep 17 00:00:00 2001 From: currantw Date: Tue, 10 Dec 2024 18:32:21 -0800 Subject: [PATCH 13/26] Fixing IT test failure. Signed-off-by: currantw --- .../src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java index b5994ed3814..8ee3e0046f4 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java @@ -49,7 +49,7 @@ public void test_cidrmatch() throws IOException { result = executeQuery( String.format( - "source=%s | where cidrmatch(host, '1.2.3.0/8') | fields host", TEST_INDEX_WEBLOG)); + "source=%s | where cidrmatch(host, '1.2.3.0/24') | fields host", TEST_INDEX_WEBLOG)); verifySchema(result, schema("host", null, "ip")); verifyDataRows(result, rows("1.2.3.4"), rows("1.2.3.5")); } From bfcd86de64c63af255f74dc3ad6d1b384d2ad768 Mon Sep 17 00:00:00 2001 From: currantw Date: Tue, 10 Dec 2024 19:12:52 -0800 Subject: [PATCH 14/26] Spotless and update test to sort in SQL Signed-off-by: currantw --- .../src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java | 3 ++- .../src/test/java/org/opensearch/sql/ppl/SortCommandIT.java | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java index 8ee3e0046f4..2d4f3837b85 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java @@ -49,7 +49,8 @@ public void test_cidrmatch() throws IOException { result = executeQuery( String.format( - "source=%s | where cidrmatch(host, '1.2.3.0/24') | fields host", TEST_INDEX_WEBLOG)); + "source=%s | where cidrmatch(host, '1.2.3.0/24') | fields host", + TEST_INDEX_WEBLOG)); verifySchema(result, schema("host", null, "ip")); verifyDataRows(result, rows("1.2.3.4"), rows("1.2.3.5")); } diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java index d3dae856c76..d5de24cbf61 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java @@ -135,7 +135,7 @@ public void testSortStringField() throws IOException { @Test public void testSortIpField() throws IOException { final JSONObject result = - executeQuery(String.format("source=%s | sort host | fields host", TEST_INDEX_WEBLOG)); + executeQuery(String.format("source=%s | fields host | sort host", TEST_INDEX_WEBLOG)); verifyOrder( result, rows("::1"), From 9cd98171952a7cb6f89af20c1d28c4d795274e3a Mon Sep 17 00:00:00 2001 From: currantw Date: Wed, 11 Dec 2024 09:32:56 -0800 Subject: [PATCH 15/26] Fix broken link Signed-off-by: currantw --- DEVELOPER_GUIDE.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEVELOPER_GUIDE.rst b/DEVELOPER_GUIDE.rst index c0d2f85668a..471890c9cda 100644 --- a/DEVELOPER_GUIDE.rst +++ b/DEVELOPER_GUIDE.rst @@ -405,7 +405,7 @@ Sample test class: Doctest >>>>>>> -Python doctest library makes our document executable which keeps it up-to-date to source code. The doc generator aforementioned served as scaffolding and generated many docs in short time. Now the examples inside is changed to doctest gradually. For more details please read `Doctest <./dev/Doctest.md>`_. +Python doctest library makes our document executable which keeps it up-to-date to source code. The doc generator aforementioned served as scaffolding and generated many docs in short time. Now the examples inside is changed to doctest gradually. For more details please read `Doctest <./docs/dev/testing-doctest.md>`_. Backports From 8c52d2b9094942d32304fa2784c0823cc8273931 Mon Sep 17 00:00:00 2001 From: currantw Date: Wed, 11 Dec 2024 10:12:44 -0800 Subject: [PATCH 16/26] Fix failing code coverage Signed-off-by: currantw --- .../opensearch/storage/script/filter/lucene/LuceneQuery.java | 5 +---- .../storage/script/filter/lucene/RangeQueryTest.java | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java index 36ebb92253a..26ef56e5766 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LuceneQuery.java @@ -215,10 +215,7 @@ private ExprValue cast(FunctionExpression castFunction, ReferenceExpression ref) .put( BuiltinFunctionName.CAST_TO_IP.getName(), (expr, ref) -> { - ExprValue value = expr.valueOf(); - return value.type().equals(ExprCoreType.IP) - ? value - : new ExprIpValue(value.stringValue()); + return new ExprIpValue(expr.valueOf().stringValue()); }) .put( BuiltinFunctionName.CAST_TO_DATE.getName(), diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/RangeQueryTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/RangeQueryTest.java index 2f5482171db..55272d4cd7f 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/RangeQueryTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/RangeQueryTest.java @@ -85,8 +85,8 @@ void test_date_has_format() { } @Test - void test_non_date_field_type() { - String dateString = "2021-11-08"; + void test_string_field_type() { + String dateString = "STRING"; OpenSearchDateType dateType = OpenSearchDateType.of(STRING); ExprValue literal = ExprValueUtils.stringValue(dateString); assertNotNull(new RangeQuery(Comparison.LT).doBuild("string_value", dateType, literal)); From 0278f0bbd28d60c245aa86422cd7a697f2305cf7 Mon Sep 17 00:00:00 2001 From: currantw Date: Wed, 11 Dec 2024 10:56:43 -0800 Subject: [PATCH 17/26] Fix failing doctest Signed-off-by: currantw --- docs/user/ppl/functions/ip.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/user/ppl/functions/ip.rst b/docs/user/ppl/functions/ip.rst index eba1afa4153..bd0bc40b0ca 100644 --- a/docs/user/ppl/functions/ip.rst +++ b/docs/user/ppl/functions/ip.rst @@ -22,7 +22,7 @@ Return type: BOOLEAN Example: - os> source=weblogs | where cidrmatch(host, '1.0.0.0/24') | fields host, url + os> source=weblogs | where cidrmatch(host, '1.2.3.0/24') | fields host, url fetched rows / total rows = 2/2 +----------------+--------------------| | host | url | From f0d4f6d034d39cf48b92de611041960555833251 Mon Sep 17 00:00:00 2001 From: currantw Date: Thu, 12 Dec 2024 16:20:17 -0800 Subject: [PATCH 18/26] Fix failing `ip.rst` doctest Signed-off-by: currantw --- docs/user/ppl/functions/ip.rst | 14 +++++++------- doctest/test_data/weblogs.json | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/user/ppl/functions/ip.rst b/docs/user/ppl/functions/ip.rst index bd0bc40b0ca..425bbd9c4a2 100644 --- a/docs/user/ppl/functions/ip.rst +++ b/docs/user/ppl/functions/ip.rst @@ -22,14 +22,14 @@ Return type: BOOLEAN Example: - os> source=weblogs | where cidrmatch(host, '1.2.3.0/24') | fields host, url + os> source=weblogs | where cidrmatch(host, '1.2.3.0/24') | fields host fetched rows / total rows = 2/2 - +----------------+--------------------| - | host | url | - +----------------+--------------------+ - | 1.2.3.4 | /history/voyager1/ | - | 1.2.3.5 | /history/voyager2/ | - +----------------+--------------------| + +---------+--------------------| + | host | url | + +---------+--------------------+ + | 1.2.3.4 | /history/voyager1/ | + | 1.2.3.5 | /history/voyager2/ | + +---------+--------------------| Note: - `ip` can be an IPv4 or an IPv6 address diff --git a/doctest/test_data/weblogs.json b/doctest/test_data/weblogs.json index 67e322e0392..fbfc8d417b8 100644 --- a/doctest/test_data/weblogs.json +++ b/doctest/test_data/weblogs.json @@ -7,6 +7,6 @@ {"index":{}} {"host": "::FFFF:1.2.3.4", "method": "GET", "url": "/history/voyager1/", "response": "200", "bytes": "1234"} {"index":{}} -{"host": "1.2.3.4", "method": "GET", "url": "/history/voyager2/", "response": "200", "bytes": "4321"} +{"host": "1.2.3.5", "method": "GET", "url": "/history/voyager2/", "response": "200", "bytes": "4321"} {"index":{}} {"host": "::FFFF:1234", "method": "GET", "url": "/history/artemis/", "response": "200", "bytes": "9876"} \ No newline at end of file From 774861b1f866c0d48191a7c414b6c1a7fa8b2acc Mon Sep 17 00:00:00 2001 From: currantw Date: Fri, 13 Dec 2024 08:46:30 -0800 Subject: [PATCH 19/26] Fix test failure due to merge. Signed-off-by: currantw --- .../org/opensearch/sql/ppl/parser/AstBuilderTest.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java index adabd045051..e33a1bbbcdb 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java @@ -715,8 +715,7 @@ public void testTrendlineSort() { Optional.of( field( "test_field", - argument("asc", booleanLiteral(true)), - argument("type", nullLiteral()))), + argument("asc", booleanLiteral(true)))), computation(5, field("test_field"), "test_field_trendline", SMA))); } @@ -729,8 +728,7 @@ public void testTrendlineSortDesc() { Optional.of( field( "test_field", - argument("asc", booleanLiteral(false)), - argument("type", nullLiteral()))), + argument("asc", booleanLiteral(false)))), computation(5, field("test_field"), "test_field_trendline", SMA))); } @@ -743,8 +741,7 @@ public void testTrendlineSortAsc() { Optional.of( field( "test_field", - argument("asc", booleanLiteral(true)), - argument("type", nullLiteral()))), + argument("asc", booleanLiteral(true)))), computation(5, field("test_field"), "test_field_trendline", SMA))); } From a2ec8cf0a376069cbdc05487fccd1207f3a17dd6 Mon Sep 17 00:00:00 2001 From: currantw Date: Fri, 13 Dec 2024 09:49:20 -0800 Subject: [PATCH 20/26] Fix spotless Signed-off-by: currantw --- .../opensearch/sql/ppl/parser/AstBuilderTest.java | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java index e33a1bbbcdb..0d2541c8f92 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java @@ -712,10 +712,7 @@ public void testTrendlineSort() { "source=t | trendline sort test_field sma(5, test_field)", trendline( relation("t"), - Optional.of( - field( - "test_field", - argument("asc", booleanLiteral(true)))), + Optional.of(field("test_field", argument("asc", booleanLiteral(true)))), computation(5, field("test_field"), "test_field_trendline", SMA))); } @@ -725,10 +722,7 @@ public void testTrendlineSortDesc() { "source=t | trendline sort - test_field sma(5, test_field)", trendline( relation("t"), - Optional.of( - field( - "test_field", - argument("asc", booleanLiteral(false)))), + Optional.of(field("test_field", argument("asc", booleanLiteral(false)))), computation(5, field("test_field"), "test_field_trendline", SMA))); } @@ -738,10 +732,7 @@ public void testTrendlineSortAsc() { "source=t | trendline sort + test_field sma(5, test_field)", trendline( relation("t"), - Optional.of( - field( - "test_field", - argument("asc", booleanLiteral(true)))), + Optional.of(field("test_field", argument("asc", booleanLiteral(true)))), computation(5, field("test_field"), "test_field_trendline", SMA))); } From 7036e5f6148884326d60e6c35070f2434e86f72c Mon Sep 17 00:00:00 2001 From: currantw Date: Tue, 17 Dec 2024 09:21:27 -0800 Subject: [PATCH 21/26] Add missing `url` field Signed-off-by: currantw --- docs/user/ppl/functions/ip.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/user/ppl/functions/ip.rst b/docs/user/ppl/functions/ip.rst index 425bbd9c4a2..429713dded8 100644 --- a/docs/user/ppl/functions/ip.rst +++ b/docs/user/ppl/functions/ip.rst @@ -22,7 +22,7 @@ Return type: BOOLEAN Example: - os> source=weblogs | where cidrmatch(host, '1.2.3.0/24') | fields host + os> source=weblogs | where cidrmatch(host, '1.2.3.0/24') | fields host, url fetched rows / total rows = 2/2 +---------+--------------------| | host | url | From ac9c30080d29833d6f4fd10cac23a73bb7d4af28 Mon Sep 17 00:00:00 2001 From: currantw Date: Tue, 17 Dec 2024 10:47:38 -0800 Subject: [PATCH 22/26] Address minor review comments. Signed-off-by: currantw --- .../org/opensearch/sql/data/model/ExprIpValue.java | 4 ++-- .../opensearch/sql/expression/ip/IPFunctions.java | 13 ++++--------- .../operator/convert/TypeCastOperators.java | 4 ++-- docs/user/ppl/cmd/trendline.rst | 2 +- .../data/value/OpenSearchExprValueFactoryTest.java | 5 +---- .../script/filter/lucene/RangeQueryTest.java | 4 ++-- 6 files changed, 12 insertions(+), 20 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 index 7ca2ffe92fb..8bdbec4bb59 100644 --- a/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java +++ b/core/src/main/java/org/opensearch/sql/data/model/ExprIpValue.java @@ -14,8 +14,8 @@ public class ExprIpValue extends AbstractExprValue { private final IPAddress value; - public ExprIpValue(String s) { - value = IPUtils.toAddress(s); + public ExprIpValue(String addressString) { + value = IPUtils.toAddress(addressString); } @Override diff --git a/core/src/main/java/org/opensearch/sql/expression/ip/IPFunctions.java b/core/src/main/java/org/opensearch/sql/expression/ip/IPFunctions.java index e42c12841a1..8b3ee230147 100644 --- a/core/src/main/java/org/opensearch/sql/expression/ip/IPFunctions.java +++ b/core/src/main/java/org/opensearch/sql/expression/ip/IPFunctions.java @@ -52,14 +52,9 @@ private ExprValue exprCidrMatch(ExprValue addressExprValue, ExprValue rangeExprV IPAddress address = addressExprValue.ipValue(); IPAddress range = IPUtils.toRange(rangeExprValue.stringValue()); - if (IPUtils.compare(address, range.getLower()) < 0) { - return ExprValueUtils.LITERAL_FALSE; - } - - if (IPUtils.compare(address, range.getUpper()) > 0) { - return ExprValueUtils.LITERAL_FALSE; - } - - return ExprValueUtils.LITERAL_TRUE; + return (IPUtils.compare(address, range.getLower()) < 0) + || (IPUtils.compare(address, range.getUpper()) > 0) + ? ExprValueUtils.LITERAL_FALSE + : ExprValueUtils.LITERAL_TRUE; } } diff --git a/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperators.java b/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperators.java index 696d02dc0f8..b388f7d89ab 100644 --- a/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperators.java +++ b/core/src/main/java/org/opensearch/sql/expression/operator/convert/TypeCastOperators.java @@ -179,8 +179,8 @@ private static DefaultFunctionResolver castToBoolean() { private static DefaultFunctionResolver castToIp() { return FunctionDSL.define( BuiltinFunctionName.CAST_TO_IP.getName(), - impl(nullMissingHandling((v) -> v), IP, IP), - impl(nullMissingHandling((v) -> new ExprIpValue(v.stringValue())), IP, STRING)); + impl(nullMissingHandling((v) -> new ExprIpValue(v.stringValue())), IP, STRING), + impl(nullMissingHandling((v) -> v), IP, IP)); } private static DefaultFunctionResolver castToDate() { diff --git a/docs/user/ppl/cmd/trendline.rst b/docs/user/ppl/cmd/trendline.rst index 166a3c056fd..e6df0d7a2ce 100644 --- a/docs/user/ppl/cmd/trendline.rst +++ b/docs/user/ppl/cmd/trendline.rst @@ -23,7 +23,7 @@ Syntax * field: mandatory. The name of the field the moving average should be calculated for. * alias: optional. The name of the resulting column containing the moving average (defaults to the field name with "_trendline"). -And the moment only the Simple Moving Average (SMA) type is supported. +At the moment only the Simple Moving Average (SMA) type is supported. It is calculated like diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java index ad2f5988b91..5b9166e2b52 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java @@ -120,10 +120,9 @@ class OpenSearchExprValueFactoryTest { .put("geoV", OpenSearchDataType.of(OpenSearchDataType.MappingType.GeoPoint)) .put("binaryV", OpenSearchDataType.of(OpenSearchDataType.MappingType.Binary)) .build(); - + private static final double TOLERANCE = 1E-5; private final OpenSearchExprValueFactory exprValueFactory = new OpenSearchExprValueFactory(MAPPING, true); - private final OpenSearchExprValueFactory exprValueFactoryNoArrays = new OpenSearchExprValueFactory(MAPPING, false); @@ -761,8 +760,6 @@ public void constructIP() { tupleValue(String.format("{\"%s\":\"%s\"}", fieldIp, ipString)).get(fieldIp)); } - private static final double TOLERANCE = 1E-5; - @Test public void constructGeoPoint() { final double lat = 42.60355556; diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/RangeQueryTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/RangeQueryTest.java index 55272d4cd7f..2f5482171db 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/RangeQueryTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/RangeQueryTest.java @@ -85,8 +85,8 @@ void test_date_has_format() { } @Test - void test_string_field_type() { - String dateString = "STRING"; + void test_non_date_field_type() { + String dateString = "2021-11-08"; OpenSearchDateType dateType = OpenSearchDateType.of(STRING); ExprValue literal = ExprValueUtils.stringValue(dateString); assertNotNull(new RangeQuery(Comparison.LT).doBuild("string_value", dateType, literal)); From 6355b2d4be747c56bbf5fd10982bf67fefed29b8 Mon Sep 17 00:00:00 2001 From: currantw Date: Tue, 17 Dec 2024 18:06:57 -0800 Subject: [PATCH 23/26] Revert sort syntax changes Signed-off-by: currantw --- .../org/opensearch/sql/ast/dsl/AstDSL.java | 2 +- .../value/OpenSearchExprValueFactoryTest.java | 5 +- ppl/src/main/antlr/OpenSearchPPLLexer.g4 | 6 ++ ppl/src/main/antlr/OpenSearchPPLParser.g4 | 15 ++++- .../sql/ppl/parser/AstExpressionBuilder.java | 5 +- .../sql/ppl/utils/ArgumentFactory.java | 13 +++- .../sql/ppl/parser/AstBuilderTest.java | 23 +++++-- .../ppl/parser/AstExpressionBuilderTest.java | 60 ++++++++++++++++--- .../sql/ppl/utils/ArgumentFactoryTest.java | 23 +++---- 9 files changed, 123 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java b/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java index 62752efe52f..d9956609ecb 100644 --- a/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java +++ b/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java @@ -431,7 +431,7 @@ public static List sortOptions() { } public static List defaultSortFieldArgs() { - return exprList(argument("asc", booleanLiteral(true))); + return exprList(argument("asc", booleanLiteral(true)), argument("type", nullLiteral())); } public static Span span(UnresolvedExpression field, UnresolvedExpression value, SpanUnit unit) { diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java index 5b9166e2b52..89dfd4dbdb2 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java @@ -688,8 +688,9 @@ public void constructArrayOfIPsReturnsAll() { final String ipv6String = "2001:db7::ff00:42:8329"; assertEquals( - new ExprCollectionValue(List.of(ipValue("1.2.3.4"), ipValue("2001:db7::ff00:42:8329"))), - tupleValue("{\"ipV\":[" + "\"1.2.3.4\"," + "\"2001:db7::ff00:42:8329\"" + "]}").get("ipV")); + new ExprCollectionValue(List.of(ipValue(ipv4String), ipValue(ipv6String))), + tupleValue(String.format("{\"%s\":[\"%s\",\"%s\"]}", fieldIp, ipv4String, ipv6String)) + .get(fieldIp)); } @Test diff --git a/ppl/src/main/antlr/OpenSearchPPLLexer.g4 b/ppl/src/main/antlr/OpenSearchPPLLexer.g4 index 3b2fce4f1c6..053ec530db4 100644 --- a/ppl/src/main/antlr/OpenSearchPPLLexer.g4 +++ b/ppl/src/main/antlr/OpenSearchPPLLexer.g4 @@ -52,6 +52,12 @@ WITH: 'WITH'; // CLAUSE KEYWORDS SORTBY: 'SORTBY'; +// SORT FIELD KEYWORDS +// TODO #3180: Fix broken sort functionality +AUTO: 'AUTO'; +STR: 'STR'; +NUM: 'NUM'; + // TRENDLINE KEYWORDS SMA: 'SMA'; diff --git a/ppl/src/main/antlr/OpenSearchPPLParser.g4 b/ppl/src/main/antlr/OpenSearchPPLParser.g4 index fd18b06aa64..27f7e4014ba 100644 --- a/ppl/src/main/antlr/OpenSearchPPLParser.g4 +++ b/ppl/src/main/antlr/OpenSearchPPLParser.g4 @@ -363,7 +363,15 @@ wcFieldList ; sortField - : (PLUS | MINUS)? fieldExpression + : (PLUS | MINUS)? sortFieldExpression + ; + +sortFieldExpression + : fieldExpression + | AUTO LT_PRTHS fieldExpression RT_PRTHS + | STR LT_PRTHS fieldExpression RT_PRTHS + | IP LT_PRTHS fieldExpression RT_PRTHS + | NUM LT_PRTHS fieldExpression RT_PRTHS ; fieldExpression @@ -890,6 +898,11 @@ keywordsCanBeId | DATASOURCES // CLAUSEKEYWORDS | SORTBY + // SORT FIELD KEYWORDS + | AUTO + | STR + | IP + | NUM // ARGUMENT KEYWORDS | KEEPEMPTY | CONSECUTIVE diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java index d18d74e40a3..5a7522683a1 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java @@ -161,8 +161,11 @@ public UnresolvedExpression visitWcFieldExpression(WcFieldExpressionContext ctx) @Override public UnresolvedExpression visitSortField(SortFieldContext ctx) { + + // TODO #3180: Fix broken sort functionality return new Field( - visit(ctx.fieldExpression().qualifiedName()), ArgumentFactory.getArgumentList(ctx)); + visit(ctx.sortFieldExpression().fieldExpression().qualifiedName()), + ArgumentFactory.getArgumentList(ctx)); } /** Aggregation function. */ diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java b/ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java index cd79ccef79f..f89ecf9c6e4 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java @@ -87,10 +87,19 @@ public static List getArgumentList(DedupCommandContext ctx) { * @return the list of arguments fetched from the sort field in sort command */ public static List getArgumentList(SortFieldContext ctx) { - return List.of( + return Arrays.asList( ctx.MINUS() != null ? new Argument("asc", new Literal(false, DataType.BOOLEAN)) - : new Argument("asc", new Literal(true, DataType.BOOLEAN))); + : new Argument("asc", new Literal(true, DataType.BOOLEAN)), + ctx.sortFieldExpression().AUTO() != null + ? new Argument("type", new Literal("auto", DataType.STRING)) + : ctx.sortFieldExpression().IP() != null + ? new Argument("type", new Literal("ip", DataType.STRING)) + : ctx.sortFieldExpression().NUM() != null + ? new Argument("type", new Literal("num", DataType.STRING)) + : ctx.sortFieldExpression().STR() != null + ? new Argument("type", new Literal("str", DataType.STRING)) + : new Argument("type", new Literal(null, DataType.NULL))); } /** diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java index 0d2541c8f92..c6f4ed20447 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java @@ -29,6 +29,7 @@ import static org.opensearch.sql.ast.dsl.AstDSL.intLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.let; import static org.opensearch.sql.ast.dsl.AstDSL.map; +import static org.opensearch.sql.ast.dsl.AstDSL.nullLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.parse; import static org.opensearch.sql.ast.dsl.AstDSL.projectWithArg; import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName; @@ -432,7 +433,9 @@ public void testSortCommandWithOptions() { "source=t | sort - f1, + f2", sort( relation("t"), - field("f1", exprList(argument("asc", booleanLiteral(false)))), + field( + "f1", + exprList(argument("asc", booleanLiteral(false)), argument("type", nullLiteral()))), field("f2", defaultSortFieldArgs()))); } @@ -712,7 +715,11 @@ public void testTrendlineSort() { "source=t | trendline sort test_field sma(5, test_field)", trendline( relation("t"), - Optional.of(field("test_field", argument("asc", booleanLiteral(true)))), + Optional.of( + field( + "test_field", + argument("asc", booleanLiteral(true)), + argument("type", nullLiteral()))), computation(5, field("test_field"), "test_field_trendline", SMA))); } @@ -722,7 +729,11 @@ public void testTrendlineSortDesc() { "source=t | trendline sort - test_field sma(5, test_field)", trendline( relation("t"), - Optional.of(field("test_field", argument("asc", booleanLiteral(false)))), + Optional.of( + field( + "test_field", + argument("asc", booleanLiteral(false)), + argument("type", nullLiteral()))), computation(5, field("test_field"), "test_field_trendline", SMA))); } @@ -732,7 +743,11 @@ public void testTrendlineSortAsc() { "source=t | trendline sort + test_field sma(5, test_field)", trendline( relation("t"), - Optional.of(field("test_field", argument("asc", booleanLiteral(true)))), + Optional.of( + field( + "test_field", + argument("asc", booleanLiteral(true)), + argument("type", nullLiteral()))), computation(5, field("test_field"), "test_field_trendline", SMA))); } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java index e4cee770136..fbb25549ab8 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java @@ -32,6 +32,7 @@ import static org.opensearch.sql.ast.dsl.AstDSL.let; import static org.opensearch.sql.ast.dsl.AstDSL.longLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.not; +import static org.opensearch.sql.ast.dsl.AstDSL.nullLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.or; import static org.opensearch.sql.ast.dsl.AstDSL.projectWithArg; import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName; @@ -324,18 +325,13 @@ public void testFieldExpr() { assertEqual("source=t | sort + f", sort(relation("t"), field("f", defaultSortFieldArgs()))); } - @Test - public void testSortFieldWithPlusKeyword() { - assertEqual( - "source=t | sort + f", - sort(relation("t"), field("f", argument("asc", booleanLiteral(true))))); - } - @Test public void testSortFieldWithMinusKeyword() { assertEqual( "source=t | sort - f", - sort(relation("t"), field("f", argument("asc", booleanLiteral(false))))); + sort( + relation("t"), + field("f", argument("asc", booleanLiteral(false)), argument("type", nullLiteral())))); } @Test @@ -343,6 +339,54 @@ public void testSortFieldWithBackticks() { assertEqual("source=t | sort `f`", sort(relation("t"), field("f", defaultSortFieldArgs()))); } + @Test + public void testSortFieldWithAutoKeyword() { + assertEqual( + "source=t | sort auto(f)", + sort( + relation("t"), + field( + "f", + argument("asc", booleanLiteral(true)), + argument("type", stringLiteral("auto"))))); + } + + @Test + public void testSortFieldWithIpKeyword() { + assertEqual( + "source=t | sort ip(f)", + sort( + relation("t"), + field( + "f", + argument("asc", booleanLiteral(true)), + argument("type", stringLiteral("ip"))))); + } + + @Test + public void testSortFieldWithNumKeyword() { + assertEqual( + "source=t | sort num(f)", + sort( + relation("t"), + field( + "f", + argument("asc", booleanLiteral(true)), + argument("type", stringLiteral("num"))))); + } + + @Test + public void testSortFieldWithStrKeyword() { + assertEqual( + "source=t | sort str(f)", + sort( + relation("t"), + field( + "f", + argument("asc", booleanLiteral(true)), + argument("type", stringLiteral("str"))))); + } + @Test public void testAggFuncCallExpr() { assertEqual( diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java index f7369218778..761dbe2997b 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/utils/ArgumentFactoryTest.java @@ -81,18 +81,21 @@ public void testDedupCommandDefaultArgument() { } @Test - public void testSortCommand() { - assertEqual( - "source=t | sort field0", - sort(relation("t"), field("field0", exprList(argument("asc", booleanLiteral(true)))))); - - assertEqual( - "source=t | sort + field0", - sort(relation("t"), field("field0", exprList(argument("asc", booleanLiteral(true)))))); + public void testSortCommandDefaultArgument() { + assertEqual("source=t | sort field0", "source=t | sort field0"); + } + @Test + public void testSortFieldArgument() { assertEqual( - "source=t | sort - field0", - sort(relation("t"), field("field0", exprList(argument("asc", booleanLiteral(false)))))); + "source=t | sort - auto(field0)", + sort( + relation("t"), + field( + "field0", + exprList( + argument("asc", booleanLiteral(false)), + argument("type", stringLiteral("auto")))))); } @Test From b8bf9b86d90e690d86b057a7ab565c80787f3a85 Mon Sep 17 00:00:00 2001 From: currantw Date: Tue, 17 Dec 2024 18:55:33 -0800 Subject: [PATCH 24/26] Minor doc update Signed-off-by: currantw --- DEVELOPER_GUIDE.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEVELOPER_GUIDE.rst b/DEVELOPER_GUIDE.rst index 471890c9cda..ec00c587a6f 100644 --- a/DEVELOPER_GUIDE.rst +++ b/DEVELOPER_GUIDE.rst @@ -405,7 +405,7 @@ Sample test class: Doctest >>>>>>> -Python doctest library makes our document executable which keeps it up-to-date to source code. The doc generator aforementioned served as scaffolding and generated many docs in short time. Now the examples inside is changed to doctest gradually. For more details please read `Doctest <./docs/dev/testing-doctest.md>`_. +Python doctest library makes our document executable which keeps it up-to-date to source code. The doc generator aforementioned served as scaffolding and generated many docs in short time. Now the examples inside is changed to doctest gradually. For more details please read `testing-doctest <./docs/dev/testing-doctest.md>`_. Backports From 5298b38a0db1296129d08052d22e5a00d08df852 Mon Sep 17 00:00:00 2001 From: currantw Date: Wed, 18 Dec 2024 12:18:53 -0800 Subject: [PATCH 25/26] FIx failing `ip.rst` doctest Signed-off-by: currantw --- docs/user/ppl/functions/ip.rst | 17 ++++++++--------- doctest/test_data/weblogs.json | 18 ++++++------------ doctest/test_mapping/weblogs.json | 21 +++++++++++++++++++++ 3 files changed, 35 insertions(+), 21 deletions(-) create mode 100644 doctest/test_mapping/weblogs.json diff --git a/docs/user/ppl/functions/ip.rst b/docs/user/ppl/functions/ip.rst index 429713dded8..30cb9020b01 100644 --- a/docs/user/ppl/functions/ip.rst +++ b/docs/user/ppl/functions/ip.rst @@ -20,20 +20,19 @@ Argument type: STRING, STRING Return type: BOOLEAN -Example: +Example:: - os> source=weblogs | where cidrmatch(host, '1.2.3.0/24') | fields host, url + > source=weblogs | where cidrmatch(host, '1.2.3.0/24') | fields host, url fetched rows / total rows = 2/2 - +---------+--------------------| - | host | url | +---------+--------------------+ + | host | url | + |---------|--------------------| | 1.2.3.4 | /history/voyager1/ | | 1.2.3.5 | /history/voyager2/ | - +---------+--------------------| + +---------+--------------------+ Note: - - `ip` can be an IPv4 or an IPv6 address - - `cidr` can be an IPv4 or an IPv6 block - - `ip` and `cidr` must be either both IPv4 or both IPv6 - - `ip` and `cidr` must both be valid and non-empty/non-null + - `ip` can be an IPv4 or IPv6 address + - `cidr` can be an IPv4 or IPv6 block + - `ip` and `cidr` must both be valid and non-missing/non-null diff --git a/doctest/test_data/weblogs.json b/doctest/test_data/weblogs.json index fbfc8d417b8..afb1679e223 100644 --- a/doctest/test_data/weblogs.json +++ b/doctest/test_data/weblogs.json @@ -1,12 +1,6 @@ -{"index":{}} -{"host": "::1", "method": "GET", "url": "/history/apollo/", "response": "200", "bytes": "6245"} -{"index":{}} -{"host": "0.0.0.2", "method": "GET", "url": "/shuttle/missions/sts-73/mission-sts-73.html", "response": "200", "bytes": "4085"} -{"index":{}} -{"host": "::3", "method": "GET", "url": "/shuttle/countdown/countdown.html", "response": "200", "bytes": "3985"} -{"index":{}} -{"host": "::FFFF:1.2.3.4", "method": "GET", "url": "/history/voyager1/", "response": "200", "bytes": "1234"} -{"index":{}} -{"host": "1.2.3.5", "method": "GET", "url": "/history/voyager2/", "response": "200", "bytes": "4321"} -{"index":{}} -{"host": "::FFFF:1234", "method": "GET", "url": "/history/artemis/", "response": "200", "bytes": "9876"} \ No newline at end of file +{"host":"::1","method":"GET","url":"/history/apollo/","response":"200","bytes":"6245"} +{"host":"0.0.0.2","method":"GET","url":"/shuttle/missions/sts-73/mission-sts-73.html","response":"200","bytes":"4085"} +{"host":"::3","method":"GET","url":"/shuttle/countdown/countdown.html","response":"200","bytes":"3985"} +{"host":"::FFFF:1.2.3.4","method":"GET","url":"/history/voyager1/","response":"200","bytes":"1234"} +{"host":"1.2.3.5","method":"GET","url":"/history/voyager2/","response": "200","bytes":"4321"} +{"host":"::FFFF:1234","method":"GET","url":"/history/artemis/","response":"200","bytes": "9876"} diff --git a/doctest/test_mapping/weblogs.json b/doctest/test_mapping/weblogs.json new file mode 100644 index 00000000000..05b9784313a --- /dev/null +++ b/doctest/test_mapping/weblogs.json @@ -0,0 +1,21 @@ +{ + "mappings": { + "properties": { + "host": { + "type": "ip" + }, + "method": { + "type": "text" + }, + "url": { + "type": "text" + }, + "response": { + "type": "text" + }, + "bytes": { + "type": "text" + } + } + } +} From 4718848196dd3281eefa2a02e41ed6b99df7c0b4 Mon Sep 17 00:00:00 2001 From: currantw Date: Wed, 18 Dec 2024 15:07:06 -0800 Subject: [PATCH 26/26] Add `IPComparisonIT` tests for comparison operators, rename modules and weblogs test index to make plural for consistency. Signed-off-by: currantw --- ...FunctionTest.java => IPFunctionsTest.java} | 2 +- .../org/opensearch/sql/legacy/JdbcTestIT.java | 2 +- .../sql/legacy/RestIntegTestCase.java | 4 +- .../sql/legacy/SQLIntegTestCase.java | 4 +- .../opensearch/sql/legacy/TestsConstants.java | 2 +- .../opensearch/sql/ppl/IPComparisonIT.java | 145 ++++++++++++++++++ .../{IPFunctionIT.java => IPFunctionsIT.java} | 10 +- .../org/opensearch/sql/ppl/SortCommandIT.java | 4 +- 8 files changed, 159 insertions(+), 14 deletions(-) rename core/src/test/java/org/opensearch/sql/expression/ip/{IPFunctionTest.java => IPFunctionsTest.java} (99%) create mode 100644 integ-test/src/test/java/org/opensearch/sql/ppl/IPComparisonIT.java rename integ-test/src/test/java/org/opensearch/sql/ppl/{IPFunctionIT.java => IPFunctionsIT.java} (89%) diff --git a/core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionTest.java b/core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionsTest.java similarity index 99% rename from core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionTest.java rename to core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionsTest.java index 8cb5ca33aeb..a74bbda3a16 100644 --- a/core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionTest.java +++ b/core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionsTest.java @@ -25,7 +25,7 @@ import org.opensearch.sql.expression.env.Environment; @ExtendWith(MockitoExtension.class) -public class IPFunctionTest { +public class IPFunctionsTest { // IP range and address constants for testing. private static final ExprValue IPv4Range = ExprValueUtils.stringValue("198.51.100.0/24"); diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/JdbcTestIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/JdbcTestIT.java index b937eea5b32..4ad88c632ba 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/JdbcTestIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/JdbcTestIT.java @@ -155,7 +155,7 @@ public void dateFunctionNameCaseInsensitiveTest() { public void ipTypeShouldPassJdbcFormatter() { assertThat( executeQuery( - "SELECT host FROM " + TestsConstants.TEST_INDEX_WEBLOG + " ORDER BY host", "jdbc"), + "SELECT host FROM " + TestsConstants.TEST_INDEX_WEBLOGS + " ORDER BY host", "jdbc"), containsString("\"type\": \"ip\"")); } diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/RestIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/RestIntegTestCase.java index a94047c1e4c..3d53b966683 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/RestIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/RestIntegTestCase.java @@ -273,8 +273,8 @@ public enum Index { getOrderIndexMapping(), "src/test/resources/order.json"), WEBLOG( - TestsConstants.TEST_INDEX_WEBLOG, - "weblog", + TestsConstants.TEST_INDEX_WEBLOGS, + "weblogs", getWeblogsIndexMapping(), "src/test/resources/weblogs.json"), DATE( diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java index 5b956fb5d39..1728be74e6c 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java @@ -661,8 +661,8 @@ public enum Index { getOrderIndexMapping(), "src/test/resources/order.json"), WEBLOG( - TestsConstants.TEST_INDEX_WEBLOG, - "weblog", + TestsConstants.TEST_INDEX_WEBLOGS, + "weblogs", getWeblogsIndexMapping(), "src/test/resources/weblogs.json"), DATE( diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/TestsConstants.java b/integ-test/src/test/java/org/opensearch/sql/legacy/TestsConstants.java index 73838feb4f0..1e336f544e9 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/TestsConstants.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/TestsConstants.java @@ -43,7 +43,7 @@ public class TestsConstants { public static final String TEST_INDEX_BANK_CSV_SANITIZE = TEST_INDEX_BANK + "_csv_sanitize"; public static final String TEST_INDEX_BANK_RAW_SANITIZE = TEST_INDEX_BANK + "_raw_sanitize"; public static final String TEST_INDEX_ORDER = TEST_INDEX + "_order"; - public static final String TEST_INDEX_WEBLOG = TEST_INDEX + "_weblog"; + public static final String TEST_INDEX_WEBLOGS = TEST_INDEX + "_weblogs"; public static final String TEST_INDEX_DATE = TEST_INDEX + "_date"; public static final String TEST_INDEX_DATE_TIME = TEST_INDEX + "_datetime"; public static final String TEST_INDEX_DEEP_NESTED = TEST_INDEX + "_deep_nested"; diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/IPComparisonIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/IPComparisonIT.java new file mode 100644 index 00000000000..a19ea32a688 --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/IPComparisonIT.java @@ -0,0 +1,145 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.ppl; + +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_WEBLOGS; +import static org.opensearch.sql.util.MatcherUtils.rows; +import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; + +import java.io.IOException; +import org.json.JSONObject; +import org.junit.Test; +import org.opensearch.sql.legacy.SQLIntegTestCase; + +public class IPComparisonIT extends PPLIntegTestCase { + + @Override + public void init() throws IOException { + loadIndex(SQLIntegTestCase.Index.WEBLOG); + } + + @Test + public void test_equal() throws IOException { + JSONObject result; + final String operator = "="; + + result = executeComparisonQuery(operator, "1.2.3.4"); + verifyDataRows(result, rows("1.2.3.4")); + + result = executeComparisonQuery(operator, "::ffff:1.2.3.4"); + verifyDataRows(result, rows("1.2.3.4")); + + result = executeComparisonQuery(operator, "::1"); + verifyDataRows(result, rows("::1")); + + result = executeComparisonQuery(operator, "0000:0000:0000:0000:0000:0000:0000:0001"); + verifyDataRows(result, rows("::1")); + } + + @Test + public void test_not_equal() throws IOException { + JSONObject result; + final String operator = "!="; + + result = executeComparisonQuery(operator, "1.2.3.4"); + verifyDataRows( + result, rows("::1"), rows("0.0.0.2"), rows("::3"), rows("1.2.3.5"), rows("::ffff:1234")); + + result = executeComparisonQuery(operator, "::ffff:1.2.3.4"); + verifyDataRows( + result, rows("::1"), rows("0.0.0.2"), rows("::3"), rows("1.2.3.5"), rows("::ffff:1234")); + + result = executeComparisonQuery(operator, "::1"); + verifyDataRows( + result, + rows("0.0.0.2"), + rows("::3"), + rows("1.2.3.4"), + rows("1.2.3.5"), + rows("::ffff:1234")); + + result = executeComparisonQuery(operator, "0000:0000:0000:0000:0000:0000:0000:0001"); + verifyDataRows( + result, + rows("0.0.0.2"), + rows("::3"), + rows("1.2.3.4"), + rows("1.2.3.5"), + rows("::ffff:1234")); + } + + @Test + public void test_greater_than() throws IOException { + JSONObject result; + final String operator = ">"; + + result = executeComparisonQuery(operator, "1.2.3.3"); + verifyDataRows(result, rows("1.2.3.4"), rows("1.2.3.5")); + + result = executeComparisonQuery(operator, "1.2.3.4"); + verifyDataRows(result, rows("1.2.3.5")); + + result = executeComparisonQuery(operator, "1.2.3.5"); + verifyDataRows(result); + } + + @Test + public void test_greater_than_or_equal_to() throws IOException { + JSONObject result; + final String operator = ">="; + + result = executeComparisonQuery(operator, "1.2.3.4"); + verifyDataRows(result, rows("1.2.3.4"), rows("1.2.3.5")); + + result = executeComparisonQuery(operator, "1.2.3.5"); + verifyDataRows(result, rows("1.2.3.5")); + + result = executeComparisonQuery(operator, "1.2.3.6"); + verifyDataRows(result); + } + + @Test + public void test_less_than() throws IOException { + JSONObject result; + final String operator = "<"; + + result = executeComparisonQuery(operator, "::4"); + verifyDataRows(result, rows("::1"), rows("::3")); + + result = executeComparisonQuery(operator, "::3"); + verifyDataRows(result, rows("::1")); + + result = executeComparisonQuery(operator, "::1"); + verifyDataRows(result); + } + + @Test + public void test_less_than_or_equal_to() throws IOException { + JSONObject result; + final String operator = "<="; + + result = executeComparisonQuery(operator, "::3"); + verifyDataRows(result, rows("::1"), rows("::3")); + + result = executeComparisonQuery(operator, "::1"); + verifyDataRows(result, rows("::1")); + + result = executeComparisonQuery(operator, "::0"); + verifyDataRows(result); + } + + /** + * Executes a query comparison on the weblogs test index with the given comparison operator and IP + * address string, and returns the resulting {@link JSONObject}; + */ + private JSONObject executeComparisonQuery(String comparisonOperator, String addressString) + throws IOException { + String formatString = "source=%s | where host %s '%s' | fields host"; + String query = + String.format(formatString, TEST_INDEX_WEBLOGS, comparisonOperator, addressString); + return executeQuery(query); + } +} diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionsIT.java similarity index 89% rename from integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java rename to integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionsIT.java index 2d4f3837b85..1b0dbf711cc 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionsIT.java @@ -5,7 +5,7 @@ package org.opensearch.sql.ppl; -import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_WEBLOG; +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_WEBLOGS; import static org.opensearch.sql.util.MatcherUtils.rows; import static org.opensearch.sql.util.MatcherUtils.schema; import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; @@ -15,7 +15,7 @@ import org.json.JSONObject; import org.junit.jupiter.api.Test; -public class IPFunctionIT extends PPLIntegTestCase { +public class IPFunctionsIT extends PPLIntegTestCase { @Override public void init() throws IOException { @@ -32,7 +32,7 @@ public void test_cidrmatch() throws IOException { executeQuery( String.format( "source=%s | where cidrmatch(host, '250.0.0.0/24') | fields host", - TEST_INDEX_WEBLOG)); + TEST_INDEX_WEBLOGS)); verifySchema(result, schema("host", null, "ip")); verifyDataRows(result); @@ -41,7 +41,7 @@ public void test_cidrmatch() throws IOException { executeQuery( String.format( "source=%s | where cidrmatch(host, '0.0.0.0/24') | fields host", - TEST_INDEX_WEBLOG)); + TEST_INDEX_WEBLOGS)); verifySchema(result, schema("host", null, "ip")); verifyDataRows(result, rows("0.0.0.2")); @@ -50,7 +50,7 @@ public void test_cidrmatch() throws IOException { executeQuery( String.format( "source=%s | where cidrmatch(host, '1.2.3.0/24') | fields host", - TEST_INDEX_WEBLOG)); + TEST_INDEX_WEBLOGS)); verifySchema(result, schema("host", null, "ip")); verifyDataRows(result, rows("1.2.3.4"), rows("1.2.3.5")); } diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java index d5de24cbf61..b234dd032d5 100644 --- a/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/ppl/SortCommandIT.java @@ -8,7 +8,7 @@ import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK; import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK_WITH_NULL_VALUES; import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DOG; -import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_WEBLOG; +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_WEBLOGS; import static org.opensearch.sql.util.MatcherUtils.rows; import static org.opensearch.sql.util.MatcherUtils.verifyOrder; @@ -135,7 +135,7 @@ public void testSortStringField() throws IOException { @Test public void testSortIpField() throws IOException { final JSONObject result = - executeQuery(String.format("source=%s | fields host | sort host", TEST_INDEX_WEBLOG)); + executeQuery(String.format("source=%s | fields host | sort host", TEST_INDEX_WEBLOGS)); verifyOrder( result, rows("::1"),