diff --git a/.gitignore b/.gitignore index 248a37ebeb..8ca3d0a2ab 100644 --- a/.gitignore +++ b/.gitignore @@ -22,6 +22,8 @@ src/site-server/node_modules /out/ .gradle/ build/ +gen/ +*.tokens # various IDE files .vscode diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/scope/SymbolTable.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/scope/SymbolTable.java index 234d759f11..ed7907718c 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/scope/SymbolTable.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/scope/SymbolTable.java @@ -22,6 +22,7 @@ import java.util.NavigableMap; import java.util.Optional; import java.util.TreeMap; +import java.util.stream.Collectors; import static java.util.Collections.emptyMap; import static java.util.Collections.emptyNavigableMap; @@ -31,8 +32,10 @@ */ public class SymbolTable { - /** Two-dimension hash table to manage symbols with type in different namespace */ - private Map> tableByNamespace = new EnumMap<>(Namespace.class); + /** + * Two-dimension hash table to manage symbols with type in different namespace + */ + private Map> tableByNamespace = new EnumMap<>(Namespace.class); /** * Store symbol with the type. Create new map for namespace for the first time. @@ -41,9 +44,12 @@ public class SymbolTable { */ public void store(Symbol symbol, Type type) { tableByNamespace.computeIfAbsent( - symbol.getNamespace(), - ns -> new TreeMap<>() - ).put(symbol.getName(), type); + symbol.getNamespace(), + ns -> new TreeMap<>() + ).computeIfAbsent( + symbol.getName(), + symbolName -> new TypeSupplier(symbolName, type) + ).add(type); } /** @@ -52,12 +58,12 @@ public void store(Symbol symbol, Type type) { * @return symbol type which is optional */ public Optional lookup(Symbol symbol) { - Map table = tableByNamespace.get(symbol.getNamespace()); - Type type = null; + Map table = tableByNamespace.get(symbol.getNamespace()); + TypeSupplier typeSupplier = null; if (table != null) { - type = table.get(symbol.getName()); + typeSupplier = table.get(symbol.getName()); } - return Optional.ofNullable(type); + return Optional.ofNullable(typeSupplier).map(TypeSupplier::get); } /** @@ -66,9 +72,12 @@ public Optional lookup(Symbol symbol) { * @return symbols starting with the prefix */ public Map lookupByPrefix(Symbol prefix) { - NavigableMap table = tableByNamespace.get(prefix.getNamespace()); + NavigableMap table = tableByNamespace.get(prefix.getNamespace()); if (table != null) { - return table.subMap(prefix.getName(), prefix.getName() + Character.MAX_VALUE); + return table.subMap(prefix.getName(), prefix.getName() + Character.MAX_VALUE) + .entrySet().stream() + .filter(entry -> null != entry.getValue().get()) + .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().get())); } return emptyMap(); } @@ -79,7 +88,10 @@ public Map lookupByPrefix(Symbol prefix) { * @return all symbols in the namespace map */ public Map lookupAll(Namespace namespace) { - return tableByNamespace.getOrDefault(namespace, emptyNavigableMap()); + return tableByNamespace.getOrDefault(namespace, emptyNavigableMap()) + .entrySet().stream() + .filter(entry -> null != entry.getValue().get()) + .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().get())); } /** diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/scope/TypeSupplier.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/scope/TypeSupplier.java new file mode 100644 index 0000000000..e1605c6f4c --- /dev/null +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/scope/TypeSupplier.java @@ -0,0 +1,61 @@ +/* + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package com.amazon.opendistroforelasticsearch.sql.antlr.semantic.scope; + +import com.amazon.opendistroforelasticsearch.sql.antlr.semantic.SemanticAnalysisException; +import com.amazon.opendistroforelasticsearch.sql.antlr.semantic.types.Type; + +import java.util.HashSet; +import java.util.Set; +import java.util.function.Supplier; + +/** + * The TypeSupplier is construct by the symbolName and symbolType. + * The TypeSupplier implement the {@link Supplier} interface to provide the {@link Type}. + * The TypeSupplier maintain types to track different {@link Type} definition for the same symbolName. + */ +public class TypeSupplier implements Supplier { + private final String symbolName; + private final Type symbolType; + private final Set types; + + public TypeSupplier(String symbolName, Type symbolType) { + this.symbolName = symbolName; + this.symbolType = symbolType; + this.types = new HashSet<>(); + this.types.add(symbolType); + } + + public TypeSupplier add(Type type) { + types.add(type); + return this; + } + + /** + * Get the {@link Type} + * Throw {@link SemanticAnalysisException} if conflict found. + * Currently, if the two types not equal, they are treated as conflicting. + */ + @Override + public Type get() { + if (types.size() > 1) { + throw new SemanticAnalysisException( + String.format("Field [%s] have conflict type [%s]", symbolName, types)); + } else { + return symbolType; + } + } +} diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/visitor/ESMappingLoader.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/visitor/ESMappingLoader.java index 34c1ebc801..4ac080006b 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/visitor/ESMappingLoader.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/visitor/ESMappingLoader.java @@ -30,6 +30,8 @@ import com.amazon.opendistroforelasticsearch.sql.utils.StringUtils; import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; import static com.amazon.opendistroforelasticsearch.sql.antlr.semantic.types.base.ESIndex.IndexType.INDEX; import static com.amazon.opendistroforelasticsearch.sql.antlr.semantic.types.base.ESIndex.IndexType.NESTED_FIELD; @@ -104,8 +106,8 @@ private void defineIndexType(String indexName) { } private void loadAllFieldsWithType(String indexName) { - FieldMappings mappings = getFieldMappings(indexName); - mappings.flat(this::defineFieldName); + Set mappings = getFieldMappings(indexName); + mappings.forEach(mapping -> mapping.flat(this::defineFieldName)); } /* @@ -139,8 +141,9 @@ private void loadAllFieldsWithType(String indexName) { * 'accounts.projects.active' -> BOOLEAN */ private void defineAllFieldNamesByAppendingAliasPrefix(String indexName, String alias) { - FieldMappings mappings = getFieldMappings(indexName); - mappings.flat((fieldName, type) -> defineFieldName(alias + "." + fieldName, type)); + Set mappings = getFieldMappings(indexName); + mappings.stream().forEach(mapping -> mapping.flat((fieldName, type) -> + defineFieldName(alias + "." + fieldName, type))); } /* @@ -177,16 +180,20 @@ private boolean isNotNested(String indexName) { return indexName.indexOf('.', 1) == -1; // taking care of .kibana } - private FieldMappings getFieldMappings(String indexName) { + private Set getFieldMappings(String indexName) { IndexMappings indexMappings = clusterState.getFieldMappings(new String[]{indexName}); - FieldMappings fieldMappings = indexMappings.firstMapping().firstMapping(); - - int size = fieldMappings.data().size(); - if (size > threshold) { - throw new EarlyExitAnalysisException(StringUtils.format( - "Index [%s] has [%d] fields more than threshold [%d]", indexName, size, threshold)); + Set fieldMappingsSet = indexMappings.allMappings().stream(). + flatMap(typeMappings -> typeMappings.allMappings().stream()). + collect(Collectors.toSet()); + + for (FieldMappings fieldMappings : fieldMappingsSet) { + int size = fieldMappings.data().size(); + if (size > threshold) { + throw new EarlyExitAnalysisException(StringUtils.format( + "Index [%s] has [%d] fields more than threshold [%d]", indexName, size, threshold)); + } } - return fieldMappings; + return fieldMappingsSet; } private void defineFieldName(String fieldName, String type) { @@ -199,9 +206,7 @@ private void defineFieldName(String fieldName, String type) { private void defineFieldName(String fieldName, Type type) { Symbol symbol = new Symbol(Namespace.FIELD_NAME, fieldName); - if (!environment().resolve(symbol).isPresent()) { // TODO: why? add test for name shadow - environment().define(symbol, type); - } + environment().define(symbol, type); } private Environment environment() { diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/visitor/SemanticAnalyzer.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/visitor/SemanticAnalyzer.java index 71c8f3f07b..b40decbc85 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/visitor/SemanticAnalyzer.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/visitor/SemanticAnalyzer.java @@ -61,6 +61,12 @@ public Type visitSelect(List itemTypes) { return typeChecker.visitSelect(itemTypes); } + @Override + public Type visitSelectAllColumn() { + mappingLoader.visitSelectAllColumn(); + return typeChecker.visitSelectAllColumn(); + } + @Override public void visitAs(String alias, Type type) { mappingLoader.visitAs(unquoteSingleField(alias), type); diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/visitor/TypeChecker.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/visitor/TypeChecker.java index a5d3f935d2..9ac58a24b1 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/visitor/TypeChecker.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/visitor/TypeChecker.java @@ -33,6 +33,7 @@ import com.amazon.opendistroforelasticsearch.sql.antlr.semantic.types.special.Product; import com.amazon.opendistroforelasticsearch.sql.antlr.visitor.GenericSqlParseTreeVisitor; import com.amazon.opendistroforelasticsearch.sql.utils.StringUtils; +import com.google.common.collect.ImmutableList; import java.util.List; import java.util.Optional; @@ -107,11 +108,18 @@ public void endVisitQuery() { public Type visitSelect(List itemTypes) { if (itemTypes.size() == 1) { return itemTypes.get(0); + } else if (itemTypes.size() == 0) { + return visitSelectAllColumn(); } // Return product for empty (SELECT *) and #items > 1 return new Product(itemTypes); } + @Override + public Type visitSelectAllColumn() { + return resolveAllColumn(); + } + @Override public void visitAs(String alias, Type type) { defineFieldName(alias, type); @@ -218,6 +226,11 @@ private Type resolve(Symbol symbol) { throw new SemanticAnalysisException(errorMsg); } + private Type resolveAllColumn() { + environment().resolveAll(Namespace.FIELD_NAME); + return new Product(ImmutableList.of()); + } + private Environment environment() { return context.peek(); } diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/visitor/AntlrSqlParseTreeVisitor.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/visitor/AntlrSqlParseTreeVisitor.java index 76356b24d4..6cfd665e93 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/visitor/AntlrSqlParseTreeVisitor.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/visitor/AntlrSqlParseTreeVisitor.java @@ -199,7 +199,7 @@ public T visitSimpleTableName(SimpleTableNameContext ctx) { @Override public T visitTableNamePattern(TableNamePatternContext ctx) { - throw new EarlyExitAnalysisException("Exit when meeting index pattern"); + return visitor.visitIndexName(ctx.getText()); } @Override @@ -246,6 +246,11 @@ public T visitSelectElements(SelectElementsContext ctx) { collect(Collectors.toList())); } + @Override + public T visitSelectStarElement(OpenDistroSqlParser.SelectStarElementContext ctx) { + return visitor.visitSelectAllColumn(); + } + @Override public T visitSelectColumnElement(SelectColumnElementContext ctx) { return visitSelectItem(ctx.fullColumnName(), ctx.uid()); diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/visitor/GenericSqlParseTreeVisitor.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/visitor/GenericSqlParseTreeVisitor.java index db6de45033..553ba2fe84 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/visitor/GenericSqlParseTreeVisitor.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/visitor/GenericSqlParseTreeVisitor.java @@ -32,6 +32,10 @@ default T visitSelect(List items) { return defaultValue(); } + default T visitSelectAllColumn() { + return defaultValue(); + } + default void visitAs(String alias, T type) {} default T visitIndexName(String indexName) { diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldRewriter.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldRewriter.java index e32287c156..5e7ee0cbd7 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldRewriter.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldRewriter.java @@ -35,15 +35,12 @@ import com.amazon.opendistroforelasticsearch.sql.esdomain.LocalClusterState; import com.amazon.opendistroforelasticsearch.sql.esdomain.mapping.FieldMappings; import com.amazon.opendistroforelasticsearch.sql.esdomain.mapping.IndexMappings; -import org.json.JSONObject; import java.util.ArrayDeque; import java.util.Deque; import java.util.HashMap; -import java.util.Locale; import java.util.Map; -import java.util.Set; -import java.util.stream.Collectors; +import java.util.Optional; import java.util.stream.Stream; /** @@ -122,8 +119,9 @@ public boolean visit(SQLIdentifierExpr expr) { if (isValidIdentifierForTerm(expr)) { Map source = null; if (this.filterType == TermRewriterFilter.COMMA || this.filterType == TermRewriterFilter.MULTI_QUERY) { - if (curScope().getFinalMapping().has(expr.getName())) { - source = curScope().getFinalMapping().mapping(expr.getName()); + Optional> optionalMap = curScope().resolveFieldMapping(expr.getName()); + if (optionalMap.isPresent()) { + source = optionalMap.get(); } else { return true; } @@ -253,51 +251,6 @@ private void checkMappingCompatibility(TermFieldScope scope, Map if (scope.getMapper().isEmpty()) { throw new VerificationException("Unknown index " + indexToType.keySet()); } - - Set indexMappings = curScope().getMapper().allMappings().stream(). - flatMap(typeMappings -> typeMappings.allMappings().stream()). - collect(Collectors.toSet()); - - final FieldMappings fieldMappings; - - if (indexMappings.size() > 1) { - Map> mergedMapping = new HashMap<>(); - - for (FieldMappings f : indexMappings) { - Map> m = f.data(); - m.forEach((k, v) -> verifySingleFieldMapping(k, v, mergedMapping)); - } - - fieldMappings = new FieldMappings(mergedMapping); - } else { - fieldMappings = curScope().getMapper().firstMapping().firstMapping(); - } - // We need finalMapping to lookup for rewriting - curScope().setFinalMapping(fieldMappings); - } - - private void verifySingleFieldMapping(final String fieldName, final Map fieldMapping, - final Map> mergedMapping) { - - if (!mergedMapping.containsKey(fieldName)) { - mergedMapping.put(fieldName, fieldMapping); - } else { - - final Map visitedMapping = mergedMapping.get(fieldName); - // check if types are same - if (!fieldMapping.equals(visitedMapping)) { - // TODO: Merge mappings if they are compatible, for text and text/keyword to text/keyword. - - String firstFieldType = new JSONObject(fieldMapping).toString().replaceAll("\"", ""); - String secondFieldType = new JSONObject(visitedMapping).toString().replaceAll("\"", ""); - - String exceptionReason = String.format(Locale.ROOT, "Different mappings are not allowed " - + "for the same field[%s]: found [%s] and [%s] ", - fieldName, firstFieldType, secondFieldType); - - throw new VerificationException(exceptionReason); - } - } } public IndexMappings getMappings(Map indexToType) { diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldScope.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldScope.java index bcae8e593a..1286d77966 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldScope.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldScope.java @@ -17,9 +17,14 @@ import com.amazon.opendistroforelasticsearch.sql.esdomain.mapping.FieldMappings; import com.amazon.opendistroforelasticsearch.sql.esdomain.mapping.IndexMappings; +import org.json.JSONObject; import java.util.HashMap; +import java.util.Locale; import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; /** * Index Mapping information in current query being visited. @@ -52,12 +57,28 @@ public void setMapper(IndexMappings mapper) { this.mapper = mapper; } - public FieldMappings getFinalMapping() { - return this.finalMapping; + public Optional> resolveFieldMapping(String fieldName) { + Set indexMappings = mapper.allMappings().stream(). + flatMap(typeMappings -> typeMappings.allMappings().stream()). + collect(Collectors.toSet()); + Optional> resolvedMapping = + indexMappings.stream() + .filter(mapping -> mapping.has(fieldName)) + .map(mapping -> mapping.mapping(fieldName)).reduce((map1, map2) -> { + if (!map1.equals(map2)) { + // TODO: Merge mappings if they are compatible, for text and text/keyword to text/keyword. + String exceptionReason = String.format(Locale.ROOT, "Different mappings are not allowed " + + "for the same field[%s]: found [%s] and [%s] ", + fieldName, pretty(map1), pretty(map2)); + throw new VerificationException(exceptionReason); + } + return map1; + }); + return resolvedMapping; } - public void setFinalMapping(FieldMappings finalMapping) { - this.finalMapping = finalMapping; + private static String pretty(Map mapping) { + return new JSONObject(mapping).toString().replaceAll("\"", ""); } } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/SemanticAnalyzerFieldTypeTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/SemanticAnalyzerFieldTypeTest.java new file mode 100644 index 0000000000..f01450a55e --- /dev/null +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/SemanticAnalyzerFieldTypeTest.java @@ -0,0 +1,107 @@ +/* + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package com.amazon.opendistroforelasticsearch.sql.antlr.semantic; + +import org.junit.Before; +import org.junit.Test; + +import static com.amazon.opendistroforelasticsearch.sql.util.MultipleIndexClusterUtils.mockMultipleIndexEnv; + +public class SemanticAnalyzerFieldTypeTest extends SemanticAnalyzerTestBase { + @Before + public void setup() { + mockMultipleIndexEnv(); + } + + /** + * id has same type in account1 and account2. + */ + @Test + public void accessFieldTypeNotInQueryPassSemanticCheck() { + validate("SELECT id FROM account* WHERE id = 1"); + } + + /** + * address doesn't exist in account1. + */ + @Test + public void accessFieldTypeOnlyInOneIndexPassSemanticCheck() { + validate("SELECT address FROM account* WHERE id = 30"); + } + + /** + * age has different type in account1 and account2. + */ + @Test + public void accessConflictFieldTypeShouldFailSemanticCheck() { + expectValidationFailWithErrorMessages("SELECT age FROM account* WHERE age = 30", + "Field [age] have conflict type"); + } + + /** + * age has different type in account1 and account2. + */ + @Test + public void mixNonConflictTypeAndConflictFieldTypeShouldFailSemanticCheck() { + expectValidationFailWithErrorMessages("SELECT id, age FROM account* WHERE id = 1", + "Field [age] have conflict type"); + } + + /** + * age has different type in account1 and account2. + */ + @Test + public void conflictFieldTypeWithAliasShouldFailSemanticCheck() { + expectValidationFailWithErrorMessages("SELECT a.age FROM account* as a", + "Field [a.age] have conflict type"); + } + + /** + * age has different type in account1 and account2. + * Todo, the error message is not accurate. + */ + @Test + public void selectAllFieldTypeShouldFailSemanticCheck() { + expectValidationFailWithErrorMessages("SELECT * FROM account*", + "Field [account*.age] have conflict type"); + } + + /** + * age has different type in account1 and account2. + */ + @Test + public void selectAllFieldTypeWithAliasShouldFailSemanticCheck() { + expectValidationFailWithErrorMessages("SELECT a.* FROM account* as a", + "Field [a.age] have conflict type"); + } + + /** + * a.projects.name has same type in account1 and account2. + */ + @Test + public void selectNestedNoneConflictTypeShouldPassSemanticCheck() { + validate("SELECT a.projects.name FROM account* as a"); + } + + /** + * a.projects.started_year has conflict type in account1 and account2. + */ + @Test + public void selectNestedConflictTypeShouldFailSemanticCheck() { + expectValidationFailWithErrorMessages("SELECT a.projects.started_year FROM account* as a", + "Field [a.projects.started_year] have conflict type"); + } +} diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/SemanticAnalyzerFromClauseTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/SemanticAnalyzerFromClauseTest.java index 0a3935a2fb..2b18690dfd 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/SemanticAnalyzerFromClauseTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/SemanticAnalyzerFromClauseTest.java @@ -35,13 +35,21 @@ public void nonExistingIndexNameShouldFail() { } @Test - public void useIndexPatternShouldSkipAllCheck() { - validate("SELECT abc FROM semant* WHERE def = 1"); + public void useNotExistFieldInIndexPatternShouldFail() { + expectValidationFailWithErrorMessages( + "SELECT abc FROM semant* WHERE def = 1", + "Field [def] cannot be found or used here.", + "Did you mean [address]?" + ); } @Test - public void useIndexAndIndexPatternShouldSkipAllCheck() { - validate("SELECT abc FROM semantics, semant* WHERE def = 1"); + public void useNotExistFieldInIndexAndIndexPatternShouldFail() { + expectValidationFailWithErrorMessages( + "SELECT abc FROM semantics, semant* WHERE def = 1", + "Field [def] cannot be found or used here.", + "Did you mean [address]?" + ); } /** diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/scope/TypeSupplierTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/scope/TypeSupplierTest.java new file mode 100644 index 0000000000..82e2eb7f1e --- /dev/null +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/scope/TypeSupplierTest.java @@ -0,0 +1,54 @@ +/* + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package com.amazon.opendistroforelasticsearch.sql.antlr.semantic.scope; + +import com.amazon.opendistroforelasticsearch.sql.antlr.semantic.SemanticAnalysisException; +import com.amazon.opendistroforelasticsearch.sql.antlr.semantic.types.base.ESDataType; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +import static org.junit.Assert.assertEquals; + +public class TypeSupplierTest { + @Rule + public ExpectedException exception = ExpectedException.none(); + + @Test + public void haveOneTypeShouldPass() { + TypeSupplier age = new TypeSupplier("age", ESDataType.INTEGER); + + assertEquals(ESDataType.INTEGER, age.get()); + } + + @Test + public void addSameTypeShouldPass() { + TypeSupplier age = new TypeSupplier("age", ESDataType.INTEGER); + age.add(ESDataType.INTEGER); + + assertEquals(ESDataType.INTEGER, age.get()); + } + + @Test + public void haveTwoTypesShouldThrowException() { + TypeSupplier age = new TypeSupplier("age", ESDataType.INTEGER); + age.add(ESDataType.TEXT); + + exception.expect(SemanticAnalysisException.class); + exception.expectMessage("Field [age] have conflict type"); + age.get(); + } +} \ No newline at end of file diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TermQueryExplainIT.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TermQueryExplainIT.java index 0291da40e8..5f20b6fee2 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TermQueryExplainIT.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TermQueryExplainIT.java @@ -71,8 +71,8 @@ public void testNonResolvingIndexPattern() throws IOException { } catch (ResponseException e) { assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(RestStatus.BAD_REQUEST.getStatus())); final String entity = TestUtils.getResponseBody(e.getResponse()); - assertThat(entity, containsString("Unknown index")); - assertThat(entity, containsString("\"type\": \"VerificationException\"")); + assertThat(entity, containsString("Field [firstname] cannot be found or used here.")); + assertThat(entity, containsString("\"type\": \"SemanticAnalysisException\"")); } } @@ -109,13 +109,25 @@ public void testNonCompatibleMappings() throws IOException { } catch (ResponseException e) { assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(RestStatus.BAD_REQUEST.getStatus())); final String entity = TestUtils.getResponseBody(e.getResponse()); - assertThat(entity, containsString("dog_name")); - assertThat(entity, containsString("{type:text,fields:{keyword:{type:keyword,ignore_above:256}}}")); - assertThat(entity, containsString("{type:text,fielddata:true}")); - assertThat(entity, containsString("\"type\": \"VerificationException\"")); + assertThat(entity, containsString("Field [holdersName] have conflict type")); + assertThat(entity, containsString("\"type\": \"SemanticAnalysisException\"")); } } + /** + * The dog_name field has same type in dog and dog2 index. + * But, the holdersName field has different type. + */ + @Test + public void testNonCompatibleMappingsButTheFieldIsNotUsed() throws IOException { + String result = explainQuery( + "SELECT dog_name " + + "FROM elasticsearch-sql_test_index_dog, elasticsearch-sql_test_index_dog2 WHERE dog_name = 'dog'"); + System.out.println(result); + assertThat(result, containsString("dog_name")); + assertThat(result, containsString("_source")); + } + @Test public void testEqualFieldMappings() throws IOException { String result = explainQuery( diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TestUtils.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TestUtils.java index ac4825e058..f0c490dc39 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TestUtils.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TestUtils.java @@ -199,6 +199,10 @@ public static String getDogIndexMapping() { public static String getDogs2IndexMapping() { return "{ \"mappings\": {" + " \"properties\": {\n" + + " \"dog_name\": {\n" + + " \"type\": \"text\",\n" + + " \"fielddata\": true\n" + + " },\n"+ " \"holdersName\": {\n" + " \"type\": \"keyword\"\n" + " }"+ diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/term/TermFieldRewriterTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/term/TermFieldRewriterTest.java index d0f9b73057..be62f8a90b 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/term/TermFieldRewriterTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/term/TermFieldRewriterTest.java @@ -18,41 +18,31 @@ import com.alibaba.druid.sql.SQLUtils; import com.alibaba.druid.sql.ast.expr.SQLQueryExpr; -import com.amazon.opendistroforelasticsearch.sql.esdomain.LocalClusterState; import com.amazon.opendistroforelasticsearch.sql.rewriter.matchtoterm.TermFieldRewriter; +import com.amazon.opendistroforelasticsearch.sql.rewriter.matchtoterm.VerificationException; import com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils; import com.amazon.opendistroforelasticsearch.sql.util.SqlParserUtils; -import com.google.common.base.Charsets; -import com.google.common.io.Resources; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; -import java.io.IOException; -import java.net.URL; - -import static com.amazon.opendistroforelasticsearch.sql.util.CheckScriptContents.mockLocalClusterState; +import static com.amazon.opendistroforelasticsearch.sql.util.MultipleIndexClusterUtils.mockMultipleIndexEnv; import static org.hamcrest.MatcherAssert.assertThat; public class TermFieldRewriterTest { - private static final String TEST_MAPPING_FILE = "mappings/semantics.json"; - @Rule public ExpectedException exception = ExpectedException.none(); @Before - public void setup() throws IOException { - URL url = Resources.getResource(TEST_MAPPING_FILE); - String mappings = Resources.toString(url, Charsets.UTF_8); - LocalClusterState.state(null); - mockLocalClusterState(mappings); + public void setup() { + mockMultipleIndexEnv(); } @Test public void testFromSubqueryShouldPass() { - String sql = "SELECT t.age as a FROM (SELECT age FROM semantics WHERE employer = 'david') t"; - String expected = "SELECT t.age as a FROM (SELECT age FROM semantics WHERE employer.keyword = 'david') t"; + String sql = "SELECT t.age as a FROM (SELECT age FROM account1 WHERE address = 'sea') t"; + String expected = "SELECT t.age as a FROM (SELECT age FROM account1 WHERE address.keyword = 'sea') t"; assertThat(rewriteTerm(sql), MatcherUtils.IsEqualIgnoreCaseAndWhiteSpace.equalToIgnoreCaseAndWhiteSpace(expected)); @@ -60,13 +50,62 @@ public void testFromSubqueryShouldPass() { @Test public void testFromSubqueryWithoutTermShouldPass() { - String sql = "SELECT t.age as a FROM (SELECT age FROM semantics WHERE age = 10) t"; + String sql = "SELECT t.age as a FROM (SELECT age FROM account1 WHERE age = 10) t"; + String expected = sql; + + assertThat(rewriteTerm(sql), + MatcherUtils.IsEqualIgnoreCaseAndWhiteSpace.equalToIgnoreCaseAndWhiteSpace(expected)); + } + + @Test + public void testFieldShouldBeRewritten() { + String sql = "SELECT age FROM account1 WHERE address = 'sea'"; + String expected = "SELECT age FROM account1 WHERE address.keyword = 'sea'"; + + assertThat(rewriteTerm(sql), + MatcherUtils.IsEqualIgnoreCaseAndWhiteSpace.equalToIgnoreCaseAndWhiteSpace(expected)); + } + + @Test + public void testSelectTheFieldWithCompatibleMappingShouldPass() { + String sql = "SELECT id FROM account* WHERE id = 10"; + String expected = sql; + + assertThat(rewriteTerm(sql), + MatcherUtils.IsEqualIgnoreCaseAndWhiteSpace.equalToIgnoreCaseAndWhiteSpace(expected)); + } + + @Test + public void testSelectTheFieldOnlyInOneIndexShouldPass() { + String sql = "SELECT address FROM account*"; + String expected = sql; + + assertThat(rewriteTerm(sql), + MatcherUtils.IsEqualIgnoreCaseAndWhiteSpace.equalToIgnoreCaseAndWhiteSpace(expected)); + } + + /** + * Ideally, it should fail. There are two reasons we didn't cover it now. + * 1. The semantic check already done that. + * 2. The {@link TermFieldRewriter} didn't touch allcolumn case. + */ + @Test + public void testSelectAllFieldWithConflictMappingShouldPass() { + String sql = "SELECT * FROM account*"; String expected = sql; assertThat(rewriteTerm(sql), MatcherUtils.IsEqualIgnoreCaseAndWhiteSpace.equalToIgnoreCaseAndWhiteSpace(expected)); } + @Test + public void testSelectTheFieldWithConflictMappingShouldThrowException() { + String sql = "SELECT age FROM account* WHERE age = 10"; + exception.expect(VerificationException.class); + exception.expectMessage("Different mappings are not allowed for the same field[age]"); + rewriteTerm(sql); + } + private String rewriteTerm(String sql) { SQLQueryExpr sqlQueryExpr = SqlParserUtils.parse(sql); sqlQueryExpr.accept(new TermFieldRewriter()); diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/util/CheckScriptContents.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/util/CheckScriptContents.java index 82390e973d..72e6a5809e 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/util/CheckScriptContents.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/util/CheckScriptContents.java @@ -222,7 +222,7 @@ public static void stubMockClient(Client mockClient) { } - private static XContentParser createParser(String mappings) throws IOException { + public static XContentParser createParser(String mappings) throws IOException { return XContentType.JSON.xContent().createParser( NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, @@ -254,7 +254,7 @@ public static ClusterService mockClusterService(String mappings) { return mockService; } - private static IndexNameExpressionResolver mockIndexNameExpressionResolver() { + public static IndexNameExpressionResolver mockIndexNameExpressionResolver() { IndexNameExpressionResolver mockResolver = mock(IndexNameExpressionResolver.class); when(mockResolver.concreteIndexNames(any(), any(), any())).thenAnswer( (Answer) invocation -> { @@ -269,7 +269,7 @@ private static IndexNameExpressionResolver mockIndexNameExpressionResolver() { return mockResolver; } - private static SqlSettings mockSqlSettings() { + public static SqlSettings mockSqlSettings() { SqlSettings settings = spy(new SqlSettings()); // Force return empty list to avoid ClusterSettings be invoked which is a final class and hard to mock. diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/util/MultipleIndexClusterUtils.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/util/MultipleIndexClusterUtils.java new file mode 100644 index 0000000000..0bd2e3a91c --- /dev/null +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/util/MultipleIndexClusterUtils.java @@ -0,0 +1,208 @@ +/* + * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package com.amazon.opendistroforelasticsearch.sql.util; + +import com.amazon.opendistroforelasticsearch.sql.esdomain.LocalClusterState; +import com.google.common.collect.ImmutableMap; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.metadata.IndexMetaData; +import org.elasticsearch.cluster.metadata.MappingMetaData; +import org.elasticsearch.cluster.metadata.MetaData; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.collect.ImmutableOpenMap; + +import java.io.IOException; +import java.util.Map; + +import static com.amazon.opendistroforelasticsearch.sql.util.CheckScriptContents.createParser; +import static com.amazon.opendistroforelasticsearch.sql.util.CheckScriptContents.mockIndexNameExpressionResolver; +import static com.amazon.opendistroforelasticsearch.sql.util.CheckScriptContents.mockSqlSettings; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Test Utility which provide the cluster have 2 indices. + */ +public class MultipleIndexClusterUtils { + public final static String INDEX_ACCOUNT_1 = "account1"; + public final static String INDEX_ACCOUNT_2 = "account2"; + public final static String INDEX_ACCOUNT_ALL = "account*"; + + public static String INDEX_ACCOUNT_1_MAPPING = "{\n" + + " \"field_mappings\": {\n" + + " \"mappings\": {\n" + + " \"account1\": {\n" + + " \"properties\": {\n" + + " \"id\": {\n" + + " \"type\": \"long\"\n" + + " },\n" + + " \"address\": {\n" + + " \"type\": \"text\",\n" + + " \"fields\": {\n" + + " \"keyword\": {\n" + + " \"type\": \"keyword\"\n" + + " }\n" + + " },\n" + + " \"fielddata\": true\n" + + " },\n" + + " \"age\": {\n" + + " \"type\": \"integer\"\n" + + " },\n" + + " \"projects\": {\n" + + " \"type\": \"nested\",\n" + + " \"properties\": {\n" + + " \"name\": {\n" + + " \"type\": \"text\",\n" + + " \"fields\": {\n" + + " \"keyword\": {\n" + + " \"type\": \"keyword\"\n" + + " }\n" + + " },\n" + + " \"fielddata\": true\n" + + " },\n" + + " \"started_year\": {\n" + + " \"type\": \"int\"\n" + + " }\n" + + " }\n" + + " }\n" + + " }\n" + + " }\n" + + " },\n" + + " \"settings\": {\n" + + " \"index\": {\n" + + " \"number_of_shards\": 1,\n" + + " \"number_of_replicas\": 0,\n" + + " \"version\": {\n" + + " \"created\": \"6050399\"\n" + + " }\n" + + " }\n" + + " },\n" + + " \"mapping_version\": \"1\",\n" + + " \"settings_version\": \"1\"\n" + + " }\n" + + "}"; + + /** + * The difference with account1. + * 1. missing address. + * 2. age has different type. + * 3. projects.started_year has different type. + */ + public static String INDEX_ACCOUNT_2_MAPPING = "{\n" + + " \"field_mappings\": {\n" + + " \"mappings\": {\n" + + " \"account2\": {\n" + + " \"properties\": {\n" + + " \"id\": {\n" + + " \"type\": \"long\"\n" + + " },\n" + + " \"age\": {\n" + + " \"type\": \"long\"\n" + + " },\n" + + " \"projects\": {\n" + + " \"type\": \"nested\",\n" + + " \"properties\": {\n" + + " \"name\": {\n" + + " \"type\": \"text\",\n" + + " \"fields\": {\n" + + " \"keyword\": {\n" + + " \"type\": \"keyword\"\n" + + " }\n" + + " },\n" + + " \"fielddata\": true\n" + + " },\n" + + " \"started_year\": {\n" + + " \"type\": \"long\"\n" + + " }\n" + + " }\n" + + " }\n" + + " }\n" + + " }\n" + + " },\n" + + " \"settings\": {\n" + + " \"index\": {\n" + + " \"number_of_shards\": 1,\n" + + " \"number_of_replicas\": 0,\n" + + " \"version\": {\n" + + " \"created\": \"6050399\"\n" + + " }\n" + + " }\n" + + " },\n" + + " \"mapping_version\": \"1\",\n" + + " \"settings_version\": \"1\"\n" + + " }\n" + + "}"; + + public static void mockMultipleIndexEnv() { + mockLocalClusterState(new ImmutableMap.Builder>>() + .put(INDEX_ACCOUNT_1, buildIndexMapping(INDEX_ACCOUNT_1, INDEX_ACCOUNT_1_MAPPING)) + .put(INDEX_ACCOUNT_2, buildIndexMapping(INDEX_ACCOUNT_2, INDEX_ACCOUNT_2_MAPPING)) + .put(INDEX_ACCOUNT_ALL, buildIndexMapping(new ImmutableMap.Builder() + .put(INDEX_ACCOUNT_1, INDEX_ACCOUNT_1_MAPPING) + .put(INDEX_ACCOUNT_2, INDEX_ACCOUNT_2_MAPPING) + .build())) + .build()); + } + + public static void mockLocalClusterState(Map>> indexMapping) { + LocalClusterState.state().setClusterService(mockClusterService(indexMapping)); + LocalClusterState.state().setResolver(mockIndexNameExpressionResolver()); + LocalClusterState.state().setSqlSettings(mockSqlSettings()); + } + + + public static ClusterService mockClusterService(Map>> indexMapping) { + ClusterService mockService = mock(ClusterService.class); + ClusterState mockState = mock(ClusterState.class); + MetaData mockMetaData = mock(MetaData.class); + + when(mockService.state()).thenReturn(mockState); + when(mockState.metaData()).thenReturn(mockMetaData); + try { + for (Map.Entry>> entry : indexMapping.entrySet()) { + when(mockMetaData.findMappings(eq(new String[]{entry.getKey()}), any(), any())).thenReturn(entry.getValue()); + } + } catch (IOException e) { + throw new IllegalStateException(e); + } + return mockService; + } + + private static ImmutableOpenMap> buildIndexMapping(Map indexMapping) { + try { + ImmutableOpenMap.Builder> builder = ImmutableOpenMap.builder(); + for (Map.Entry entry : indexMapping.entrySet()) { + builder.put(entry.getKey(), IndexMetaData.fromXContent(createParser(entry.getValue())).getMappings()); + } + return builder.build(); + } catch (IOException e) { + throw new IllegalStateException(e); + } + } + + private static ImmutableOpenMap> buildIndexMapping(String index, + String mapping) { + try { + ImmutableOpenMap.Builder> builder = ImmutableOpenMap.builder(); + builder.put(index, IndexMetaData.fromXContent(createParser(mapping)).getMappings()); + return builder.build(); + } catch (IOException e) { + throw new IllegalStateException(e); + } + } +}