Skip to content

Commit

Permalink
Finalize implementation and tests.
Browse files Browse the repository at this point in the history 
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
  • Loading branch information
@Yury-Fridlyand
Yury-Fridlyand committed Jul 27, 2023
1 parent d3b66ac commit 5232ad2
Show file tree
Hide file tree
Showing 15 changed files with 356 additions and 189 deletions.
7 changes: 4 additions & 3 deletions opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public boolean shouldCast(ExprType other) {
&& ExprCoreType.numberTypes().contains(otherCoreType)) {
return false;
}
return exprCoreType == ExprCoreType.UNKNOWN || exprCoreType.shouldCast(other);
return exprCoreType == ExprCoreType.UNKNOWN || exprCoreType.shouldCast(otherCoreType);
}

/**
Expand Down Expand Up @@ -181,6 +181,7 @@ public static Map<String, OpenSearchDataType> parseMapping(Map<String, Object> i
* @param mappingType A mapping type.
* @return An instance or inheritor of `OpenSearchDataType`.
*/
@SuppressWarnings("unchecked")
public static OpenSearchDataType of(MappingType mappingType, Map<String, Object> innerMap) {
OpenSearchDataType res = instances.getOrDefault(mappingType.toString(),
new OpenSearchDataType(mappingType)
Expand All @@ -207,8 +208,8 @@ public static OpenSearchDataType of(MappingType mappingType, Map<String, Object>
case Ip: return OpenSearchIpType.of();
case Date:
// Default date formatter is used when "" is passed as the second parameter
String format = (String) innerMap.getOrDefault("format", "");
return OpenSearchDateType.of(format);
return innerMap.isEmpty() ? OpenSearchDateType.of()
: OpenSearchDateType.of((String) innerMap.getOrDefault("format", ""));
default:
return res;
}
Expand Down
21 changes: 10 additions & 11 deletions opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,6 @@ private OpenSearchDateType(ExprCoreType exprCoreType) {
this.exprCoreType = exprCoreType;
}

private OpenSearchDateType(ExprType exprType) {
this();
this.exprCoreType = (ExprCoreType) exprType;
}

private OpenSearchDateType(String format) {
super(MappingType.Date);
this.formats = getFormatList(format);
Expand Down Expand Up @@ -374,25 +369,29 @@ public static OpenSearchDateType of(String format) {
return new OpenSearchDateType(format);
}

/** A public constructor replacement. */
public static OpenSearchDateType of(ExprCoreType exprCoreType) {
if (!isDateTypeCompatible(exprCoreType)) {
throw new IllegalArgumentException(String.format("Not a date/time type: %s", exprCoreType));
}
return new OpenSearchDateType(exprCoreType);
}

/** A public constructor replacement. */
public static OpenSearchDateType of(ExprType exprType) {
return new OpenSearchDateType(exprType);
if (!isDateTypeCompatible(exprType)) {
throw new IllegalArgumentException(String.format("Not a date/time type: %s", exprType));
}
return new OpenSearchDateType((ExprCoreType) exprType);
}

public static OpenSearchDateType of() {
return OpenSearchDateType.instance;
}

@Override
public List<ExprType> getParent() {
return List.of(exprCoreType);
}

@Override
public boolean shouldCast(ExprType other) {
// TODO override to fix for https://github.com/opensearch-project/sql/issues/1847
return false;
}

Expand Down
14 changes: 1 addition & 13 deletions opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchTextType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,8 @@
import static org.opensearch.sql.data.type.ExprCoreType.STRING;

import com.google.common.collect.ImmutableMap;
import java.util.List;
import java.util.Map;
import lombok.Getter;
import org.opensearch.sql.data.type.ExprType;

/**
* The type of a text value. See
Expand Down Expand Up @@ -46,19 +44,9 @@ public static OpenSearchTextType of() {
return OpenSearchTextType.instance;
}

@Override
public List<ExprType> getParent() {
return List.of(STRING);
}

@Override
public boolean shouldCast(ExprType other) {
return false;
}

@Override
protected OpenSearchDataType cloneEmpty() {
return OpenSearchTextType.of(Map.copyOf(this.fields));
return OpenSearchTextType.of(Map.copyOf(fields));
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion opensearch/src/test/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClientTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ void get_index_mappings() throws IOException {
() -> assertEquals("KEYWORD", mapping.get("city").legacyTypeName()),
() -> assertEquals(OpenSearchTextType.of(MappingType.Keyword),
parsedTypes.get("city")),
() -> assertEquals("DATE", mapping.get("birthday").legacyTypeName()),
() -> assertEquals("TIMESTAMP", mapping.get("birthday").legacyTypeName()),
() -> assertEquals(OpenSearchTextType.of(MappingType.Date),
parsedTypes.get("birthday")),
() -> assertEquals("GEO_POINT", mapping.get("location").legacyTypeName()),
Expand Down
2 changes: 1 addition & 1 deletion opensearch/src/test/java/org/opensearch/sql/opensearch/client/OpenSearchRestClientTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ void get_index_mappings() throws IOException {
() -> assertEquals("KEYWORD", mapping.get("city").legacyTypeName()),
() -> assertEquals(OpenSearchTextType.of(MappingType.Keyword),
parsedTypes.get("city")),
() -> assertEquals("DATE", mapping.get("birthday").legacyTypeName()),
() -> assertEquals("TIMESTAMP", mapping.get("birthday").legacyTypeName()),
() -> assertEquals(OpenSearchTextType.of(MappingType.Date),
parsedTypes.get("birthday")),
() -> assertEquals("GEO_POINT", mapping.get("location").legacyTypeName()),
Expand Down
114 changes: 81 additions & 33 deletions opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataTypeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import static org.junit.jupiter.api.Assertions.assertAll;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertNull;
Expand All @@ -19,7 +20,7 @@
import static org.opensearch.sql.data.type.ExprCoreType.ARRAY;
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.DATETIME;
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;
Expand Down Expand Up @@ -55,41 +56,96 @@ class OpenSearchDataTypeTest {

private static final OpenSearchDateType dateType = OpenSearchDateType.of(emptyFormatString);

public static class TestType extends OpenSearchDataType {

public TestType(MappingType mappingType, ExprCoreType type) {
super(mappingType);
exprCoreType = type;
}
}

@Test
public void equals() {
assertAll(
() -> assertEquals(textType, textType),
() -> assertEquals(OpenSearchDataType.of(INTEGER), INTEGER),
() -> assertNotEquals(textType, 42),
() -> assertEquals(OpenSearchDataType.of(MappingType.GeoPoint),
OpenSearchDataType.of(MappingType.GeoPoint)),
() -> assertNotEquals(OpenSearchDataType.of(MappingType.GeoPoint),
OpenSearchDataType.of(MappingType.Ip)),
() -> assertEquals(OpenSearchDataType.of(STRING), OpenSearchDataType.of(STRING)),
() -> assertEquals(OpenSearchDataType.of(DOUBLE),
OpenSearchDataType.of(MappingType.Double)),
() -> assertEquals(OpenSearchDataType.of(MappingType.Double),
OpenSearchDataType.of(DOUBLE)),
() -> assertEquals(OpenSearchDataType.of(DOUBLE), OpenSearchDataType.of(DOUBLE)),
() -> assertEquals(new TestType(MappingType.Double, DOUBLE),
new TestType(MappingType.Double, DOUBLE)),
() -> assertNotEquals(new TestType(MappingType.Ip, UNKNOWN),
new TestType(MappingType.Binary, UNKNOWN)),
() -> assertNotEquals(OpenSearchDataType.of(MappingType.Date),
OpenSearchDateType.of("date")),
() -> assertNotEquals(OpenSearchDataType.of(INTEGER), OpenSearchDataType.of(DOUBLE))
);
}

@Test
public void getParent() {
assertEquals(DATETIME.getParent(), OpenSearchDataType.of(DATETIME).getParent());
}

@Test
public void isCompatible() {
assertTrue(STRING.isCompatible(textType));
assertFalse(textType.isCompatible(STRING));
assertAll(
() -> assertTrue(STRING.isCompatible(textType)),
() -> assertTrue(textType.isCompatible(STRING)),

assertTrue(STRING.isCompatible(textKeywordType));
assertTrue(textType.isCompatible(textKeywordType));
() -> assertTrue(STRING.isCompatible(textKeywordType)),
() -> assertTrue(textType.isCompatible(textKeywordType))
);
}

// `typeName` and `legacyTypeName` return different things:
// https://github.com/opensearch-project/sql/issues/1296
@Test
public void typeName() {
assertEquals("STRING", textType.typeName());
assertEquals("STRING", textKeywordType.typeName());
assertEquals("OBJECT", OpenSearchDataType.of(MappingType.Object).typeName());
assertEquals("DATE", OpenSearchDataType.of(MappingType.Date).typeName());
assertEquals("DOUBLE", OpenSearchDataType.of(MappingType.Double).typeName());
assertEquals("KEYWORD", OpenSearchDataType.of(MappingType.Keyword).typeName());
assertAll(
() -> assertEquals("STRING", textType.typeName()),
() -> assertEquals("STRING", textKeywordType.typeName()),
() -> assertEquals("OBJECT", OpenSearchDataType.of(MappingType.Object).typeName()),
() -> assertEquals("TIMESTAMP", OpenSearchDataType.of(MappingType.Date).typeName()),
() -> assertEquals("DOUBLE", OpenSearchDataType.of(MappingType.Double).typeName()),
() -> assertEquals("STRING", OpenSearchDataType.of(MappingType.Keyword).typeName())
);
}

@Test
public void legacyTypeName() {
assertEquals("TEXT", textType.legacyTypeName());
assertEquals("TEXT", textKeywordType.legacyTypeName());
assertEquals("OBJECT", OpenSearchDataType.of(MappingType.Object).legacyTypeName());
assertEquals("DATE", OpenSearchDataType.of(MappingType.Date).legacyTypeName());
assertEquals("DOUBLE", OpenSearchDataType.of(MappingType.Double).legacyTypeName());
assertEquals("KEYWORD", OpenSearchDataType.of(MappingType.Keyword).legacyTypeName());
assertAll(
() -> assertEquals("TEXT", textType.legacyTypeName()),
() -> assertEquals("TEXT", textKeywordType.legacyTypeName()),
() -> assertEquals("OBJECT", OpenSearchDataType.of(MappingType.Object).legacyTypeName()),
() -> assertEquals("TIMESTAMP", OpenSearchDataType.of(MappingType.Date).legacyTypeName()),
() -> assertEquals("DOUBLE", OpenSearchDataType.of(MappingType.Double).legacyTypeName()),
() -> assertEquals("KEYWORD", OpenSearchDataType.of(MappingType.Keyword).legacyTypeName())
);
}

@Test
public void shouldCast() {
assertFalse(textType.shouldCast(STRING));
assertFalse(textKeywordType.shouldCast(STRING));
assertAll(
() -> assertFalse(textType.shouldCast(STRING)),
() -> assertTrue(textType.shouldCast(INTEGER)),
() -> assertFalse(textKeywordType.shouldCast(STRING)),
() -> assertTrue(textType.shouldCast(() -> null)),
() -> assertFalse(OpenSearchDataType.of(MappingType.Keyword).shouldCast(STRING)),
() -> assertFalse(OpenSearchDataType.of(MappingType.Keyword).shouldCast(textType)),
() -> assertFalse(OpenSearchDataType.of(MappingType.Long).shouldCast(LONG)),
() -> assertTrue(OpenSearchDataType.of(MappingType.Long).shouldCast(STRING)),
() -> assertTrue(OpenSearchBinaryType.of().shouldCast(OpenSearchBinaryType.of())),
() -> assertTrue(OpenSearchBinaryType.of().shouldCast(STRING))
);
}

private static Stream<Arguments> getTestDataWithType() {
Expand All @@ -105,15 +161,12 @@ private static Stream<Arguments> getTestDataWithType() {
Arguments.of(MappingType.ScaledFloat, "scaled_float", DOUBLE),
Arguments.of(MappingType.Double, "double", DOUBLE),
Arguments.of(MappingType.Boolean, "boolean", BOOLEAN),
Arguments.of(MappingType.Date, "date", TIMESTAMP),
Arguments.of(MappingType.Date, "timestamp", TIMESTAMP),
Arguments.of(MappingType.Object, "object", STRUCT),
Arguments.of(MappingType.Nested, "nested", ARRAY),
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.GeoPoint, "geo_point", OpenSearchGeoPointType.of()),
Arguments.of(MappingType.Binary, "binary", OpenSearchBinaryType.of()),
Arguments.of(MappingType.Ip, "ip", OpenSearchIpType.of())
);
}

Expand All @@ -124,7 +177,7 @@ public void of_MappingType(MappingType mappingType, String name, ExprType dataTy
// For serialization of SQL and PPL different functions are used, and it was designed to return
// different types. No clue why, but it should be fixed in #1296.
var nameForSQL = name.toUpperCase();
var nameForPPL = name.equals("text") ? "STRING" : name.toUpperCase();
var nameForPPL = name.equals("text") || name.equals("keyword") ? "STRING" : name.toUpperCase();
assertAll(
() -> assertEquals(nameForPPL, type.typeName()),
() -> assertEquals(nameForSQL, type.legacyTypeName()),
Expand Down Expand Up @@ -396,7 +449,7 @@ private Map<String, OpenSearchDataType> getSampleMapping() {
}

@Test
public void test_getExprType() {
public void getExprType() {
assertEquals(OpenSearchTextType.of(),
OpenSearchDataType.of(MappingType.Text).getExprType());
assertEquals(FLOAT, OpenSearchDataType.of(MappingType.Float).getExprType());
Expand All @@ -405,9 +458,4 @@ public void test_getExprType() {
assertEquals(DOUBLE, OpenSearchDataType.of(MappingType.ScaledFloat).getExprType());
assertEquals(TIMESTAMP, OpenSearchDataType.of(MappingType.Date).getExprType());
}

@Test
public void test_shouldCastFunction() {
assertFalse(dateType.shouldCast(DATE));
}
}
35 changes: 29 additions & 6 deletions opensearch/src/test/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateTypeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import static org.opensearch.sql.data.type.ExprCoreType.DATE;
import static org.opensearch.sql.data.type.ExprCoreType.DATETIME;
import static org.opensearch.sql.data.type.ExprCoreType.STRING;
import static org.opensearch.sql.data.type.ExprCoreType.TIME;
import static org.opensearch.sql.data.type.ExprCoreType.TIMESTAMP;
import static org.opensearch.sql.opensearch.data.type.OpenSearchDateType.SUPPORTED_NAMED_DATETIME_FORMATS;
Expand All @@ -23,6 +25,7 @@
import static org.opensearch.sql.opensearch.data.type.OpenSearchDateType.isDateTypeCompatible;

import com.google.common.collect.Lists;
import java.time.Instant;
import java.util.EnumSet;
import java.util.List;
import java.util.stream.Stream;
Expand All @@ -33,6 +36,8 @@
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.opensearch.common.time.FormatNames;
import org.opensearch.sql.data.model.ExprTimestampValue;
import org.opensearch.sql.data.model.ExprValue;
import org.opensearch.sql.data.type.ExprCoreType;

@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
Expand Down Expand Up @@ -89,21 +94,21 @@ public void isCompatible() {
public void check_typeName() {
assertAll(
// always use the MappingType of "DATE"
() -> assertEquals("DATE", defaultDateType.typeName()),
() -> assertEquals("DATE", timeDateType.typeName()),
() -> assertEquals("TIMESTAMP", defaultDateType.typeName()),
() -> assertEquals("TIME", timeDateType.typeName()),
() -> assertEquals("DATE", dateDateType.typeName()),
() -> assertEquals("DATE", datetimeDateType.typeName())
() -> assertEquals("TIMESTAMP", datetimeDateType.typeName())
);
}

@Test
public void check_legacyTypeName() {
assertAll(
// always use the legacy "DATE" type
() -> assertEquals("DATE", defaultDateType.legacyTypeName()),
() -> assertEquals("DATE", timeDateType.legacyTypeName()),
() -> assertEquals("TIMESTAMP", defaultDateType.legacyTypeName()),
() -> assertEquals("TIME", timeDateType.legacyTypeName()),
() -> assertEquals("DATE", dateDateType.legacyTypeName()),
() -> assertEquals("DATE", datetimeDateType.legacyTypeName())
() -> assertEquals("TIMESTAMP", datetimeDateType.legacyTypeName())
);
}

Expand Down Expand Up @@ -261,4 +266,22 @@ public void check_if_date_type_compatible() {
assertFalse(isDateTypeCompatible(OpenSearchDataType.of(
OpenSearchDataType.MappingType.Text)));
}

@Test
public void throw_if_create_with_incompatible_type() {
assertThrows(IllegalArgumentException.class, () -> OpenSearchDateType.of(STRING));
assertThrows(IllegalArgumentException.class,
() -> OpenSearchDateType.of(OpenSearchTextType.of()));
}

@Test
public void convert_value() {
ExprValue value = new ExprTimestampValue(Instant.ofEpochMilli(42));
assertEquals(42L, OpenSearchDateType.of().convertValueForSearchQuery(value));
}

@Test
public void shouldCast() {
assertFalse(OpenSearchDateType.of().shouldCast(() -> null));
}
}
Loading

0 comments on commit 5232ad2

Please sign in to comment.