From 554a460f2ef61cbb9403de3653426eae74a95f09 Mon Sep 17 00:00:00 2001 From: Yury-Fridlyand Date: Fri, 21 Jul 2023 11:53:03 -0700 Subject: [PATCH] Code clean up and comment update. Signed-off-by: Yury-Fridlyand --- .../opensearch/sql/data/type/ExprType.java | 10 +++++--- .../sql/data/type/WideningTypeRule.java | 15 ++++-------- docs/dev/text-type.md | 7 ++++++ .../data/type/OpenSearchDataType.java | 24 ++++++++++--------- .../data/type/OpenSearchDateType.java | 2 +- .../data/type/OpenSearchTextType.java | 8 ++----- .../opensearch/storage/OpenSearchIndex.java | 2 +- .../dsl/AggregationBuilderHelper.java | 1 - .../dsl/BucketAggregationBuilder.java | 1 - .../storage/script/core/ExpressionScript.java | 1 - .../script/filter/lucene/LikeQuery.java | 1 - .../script/filter/lucene/TermQuery.java | 2 -- .../storage/script/sort/SortQueryBuilder.java | 2 -- 13 files changed, 35 insertions(+), 41 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/data/type/ExprType.java b/core/src/main/java/org/opensearch/sql/data/type/ExprType.java index 7dd3f1a669..a02b3e1712 100644 --- a/core/src/main/java/org/opensearch/sql/data/type/ExprType.java +++ b/core/src/main/java/org/opensearch/sql/data/type/ExprType.java @@ -50,7 +50,7 @@ default boolean shouldCast(ExprType other) { * Get the parent type. */ default List getParent() { - return Arrays.asList(UNKNOWN); + return List.of(UNKNOWN); } /** @@ -65,12 +65,16 @@ default String legacyTypeName() { return typeName(); } - // TODO doc + /** + * Perform field name conversion if needed before inserting it into a search query. + */ default String convertFieldForSearchQuery(String fieldName) { return fieldName; } - // TODO doc + /** + * Perform value conversion if needed before inserting it into a search query. + */ default Object convertValueForSearchQuery(ExprValue value) { return value.value(); } diff --git a/core/src/main/java/org/opensearch/sql/data/type/WideningTypeRule.java b/core/src/main/java/org/opensearch/sql/data/type/WideningTypeRule.java index 22144783bd..c472563aba 100644 --- a/core/src/main/java/org/opensearch/sql/data/type/WideningTypeRule.java +++ b/core/src/main/java/org/opensearch/sql/data/type/WideningTypeRule.java @@ -13,15 +13,8 @@ /** * The definition of widening type rule for expression value. - * ExprType Widens to data types - * INTEGER LONG, FLOAT, DOUBLE - * LONG FLOAT, DOUBLE - * FLOAT DOUBLE - * DOUBLE DOUBLE - * STRING STRING - * BOOLEAN BOOLEAN - * ARRAY ARRAY - * STRUCT STRUCT + * See type widening definitions in {@link ExprCoreType}. + * For example, SHORT widens BYTE and so on. */ @UtilityClass public class WideningTypeRule { @@ -53,9 +46,9 @@ private static int distance(ExprType type1, ExprType type2, int distance) { } /** - * The max type among two types. The max is defined as follow + * The max type among two types. The max is defined as follows: * if type1 could widen to type2, then max is type2, vice versa - * if type1 could't widen to type2 and type2 could't widen to type1, + * if type1 couldn't widen to type2 and type2 couldn't widen to type1, * then throw {@link ExpressionEvaluationException}. * * @param type1 type1 diff --git a/docs/dev/text-type.md b/docs/dev/text-type.md index 9fc2033e2a..0c82d5c5a2 100644 --- a/docs/dev/text-type.md +++ b/docs/dev/text-type.md @@ -36,3 +36,10 @@ The solution is to provide to `:core` non simplified types, but full types. Thos 2. Update `OpenSearchDataType` (and inheritors if needed) to be comparable with `ExprCoreType`. 3. Update `:core` to do proper comparison (use `.equals` instead of `==`). 5. Update `:opensearch` to use the mapping information received from `:core` and properly build the search query. + +## Type Schema + +| JDBC type | `ExprCoreType` | `OpenSearchDataType` | OpenSearch type | +| --- | --- | --- | --- | +| `VARCHAR`/`CHAR` | `STRING` | -- | `keyword` | +| `LONGVARCHAR`/`TEXT` | `STRING` | `OpenSearchTextType` | `text` | 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 c4fc5c5390..b84102a119 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 @@ -17,12 +17,17 @@ import org.apache.commons.lang3.EnumUtils; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; +import org.opensearch.sql.data.type.WideningTypeRule; /** * The extension of ExprType in OpenSearch. */ public class OpenSearchDataType implements ExprType, Serializable { + /** + * Redefine comparison operation: class (or derived) could be compared with ExprCoreType too. + * Used in {@link WideningTypeRule#distance(ExprType, ExprType)}. + */ public boolean equals(final Object o) { if (o == this) { return true; @@ -46,17 +51,17 @@ public int hashCode() { @Override public List getParent() { - return exprCoreType == ExprCoreType.UNKNOWN - ? List.of(ExprCoreType.UNKNOWN) - : exprCoreType.getParent(); + return exprCoreType.getParent(); } @Override public boolean shouldCast(ExprType other) { - ExprCoreType otherCoreType = other instanceof ExprCoreType ? (ExprCoreType) other - : (other instanceof OpenSearchDataType - ? ((OpenSearchDataType) other).exprCoreType : ExprCoreType.UNKNOWN); - // TODO Copied from BuiltinFunctionRepository.isCastRequired + ExprCoreType otherCoreType = ExprCoreType.UNKNOWN; + if (other instanceof ExprCoreType) { + otherCoreType = (ExprCoreType) other; + } else if (other instanceof OpenSearchDataType) { + otherCoreType = ((OpenSearchDataType) other).exprCoreType; + } if (ExprCoreType.numberTypes().contains(exprCoreType) && ExprCoreType.numberTypes().contains(otherCoreType)) { return false; @@ -163,9 +168,6 @@ public static Map parseMapping(Map i return; } // create OpenSearchDataType - - // TODO parse `fielddata` - result.put(k, OpenSearchDataType.of( EnumUtils.getEnumIgnoreCase(OpenSearchDataType.MappingType.class, type), innerMap) @@ -196,7 +198,7 @@ public static OpenSearchDataType of(MappingType mappingType, Map objectDataType.properties = properties; return objectDataType; case Text: - // TODO update these 2 below #1038 https://github.com/opensearch-project/sql/issues/1038 + // don't parse `fielddata`, because it does not contain any info which could be used by SQL Map fields = parseMapping((Map) innerMap.getOrDefault("fields", Map.of())); return (!fields.isEmpty()) ? OpenSearchTextType.of(fields) : OpenSearchTextType.of(); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java index 384b9e6b88..360c273945 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDateType.java @@ -416,7 +416,7 @@ public String legacyTypeName() { @Override public Object convertValueForSearchQuery(ExprValue value) { - // TODO fix for https://github.com/opensearch-project/sql/issues/1847 + // TODO add here fix for https://github.com/opensearch-project/sql/issues/1847 return value.timestampValue().toEpochMilli(); } } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchTextType.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchTextType.java index 012b001245..b5a9ddb940 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchTextType.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchTextType.java @@ -63,15 +63,11 @@ protected OpenSearchDataType cloneEmpty() { @Override public String convertFieldForSearchQuery(String fieldName) { - if (fields.size() > 1) { - // TODO or pick first? - throw new RuntimeException("too many text fields"); - } if (fields.size() == 0) { return fieldName; } - // TODO what if field is not a keyword - // https://github.com/opensearch-project/sql/issues/1112 + // Pick first field. What to do if there are multiple fields? + // Multi-field text support requested in https://github.com/opensearch-project/sql/issues/1887 return String.format("%s.%s", fieldName, fields.keySet().toArray()[0]); } } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java index 70055e9dfc..f41e9dc732 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java @@ -127,7 +127,7 @@ public Map getFieldTypes() { cachedFieldTypes = OpenSearchDataType.traverseAndFlatten(cachedFieldOpenSearchTypes) .entrySet().stream().collect( LinkedHashMap::new, - (map, item) -> map.put(item.getKey(), item.getValue()/*.getExprType()*/), + (map, item) -> map.put(item.getKey(), item.getValue()), Map::putAll); } return cachedFieldTypes; diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/AggregationBuilderHelper.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/AggregationBuilderHelper.java index a914ce757e..f93f5b37ca 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/AggregationBuilderHelper.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/AggregationBuilderHelper.java @@ -17,7 +17,6 @@ import org.opensearch.sql.expression.FunctionExpression; import org.opensearch.sql.expression.LiteralExpression; import org.opensearch.sql.expression.ReferenceExpression; -import org.opensearch.sql.opensearch.data.type.OpenSearchTextType; import org.opensearch.sql.opensearch.storage.serialization.ExpressionSerializer; /** diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilder.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilder.java index 8a2638ae85..44922c9b6c 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilder.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilder.java @@ -13,7 +13,6 @@ import com.google.common.collect.ImmutableList; import java.util.List; import java.util.stream.Stream; - import org.apache.commons.lang3.tuple.Triple; import org.opensearch.search.aggregations.bucket.composite.CompositeValuesSourceBuilder; import org.opensearch.search.aggregations.bucket.composite.DateHistogramValuesSourceBuilder; diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/core/ExpressionScript.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/core/ExpressionScript.java index 4dd4a4862b..4a7537e287 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/core/ExpressionScript.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/core/ExpressionScript.java @@ -29,7 +29,6 @@ import org.opensearch.sql.expression.env.Environment; import org.opensearch.sql.expression.parse.ParseExpression; import org.opensearch.sql.opensearch.data.type.OpenSearchDataType; -import org.opensearch.sql.opensearch.data.type.OpenSearchTextType; import org.opensearch.sql.opensearch.data.value.OpenSearchExprValueFactory; /** diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LikeQuery.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LikeQuery.java index 41a9a2d7f8..bb796bee9f 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LikeQuery.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/LikeQuery.java @@ -10,7 +10,6 @@ import org.opensearch.index.query.WildcardQueryBuilder; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.type.ExprType; -import org.opensearch.sql.opensearch.data.type.OpenSearchTextType; import org.opensearch.sql.opensearch.storage.script.StringUtils; public class LikeQuery extends LuceneQuery { diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/TermQuery.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/TermQuery.java index 242402d30c..14fcbfbcc7 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/TermQuery.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/filter/lucene/TermQuery.java @@ -9,9 +9,7 @@ import org.opensearch.index.query.QueryBuilder; import org.opensearch.index.query.QueryBuilders; import org.opensearch.sql.data.model.ExprValue; -import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; -import org.opensearch.sql.opensearch.data.type.OpenSearchTextType; /** * Lucene query that build term query for equality comparison. diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/sort/SortQueryBuilder.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/sort/SortQueryBuilder.java index 22848036e1..bdee6bac96 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/sort/SortQueryBuilder.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/sort/SortQueryBuilder.java @@ -20,8 +20,6 @@ import org.opensearch.sql.expression.Expression; import org.opensearch.sql.expression.FunctionExpression; import org.opensearch.sql.expression.ReferenceExpression; -import org.opensearch.sql.expression.function.BuiltinFunctionName; -import org.opensearch.sql.opensearch.data.type.OpenSearchTextType; /** * Builder of {@link SortBuilder}.