Enhancement, add the field type conflict check in semantic check (#470)

* Enhancement, add the field type conflict check in semantic check

.gitignore

@@ -22,6 +22,8 @@ src/site-server/node_modules
 /out/
 .gradle/
 build/
+gen/
+*.tokens
 
 # various IDE files
 .vscode

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<Namespace, NavigableMap<String, Type>> tableByNamespace = new EnumMap<>(Namespace.class);
+    /**
+     * Two-dimension hash table to manage symbols with type in different namespace
+     */
+    private Map<Namespace, NavigableMap<String, TypeSupplier>> 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<Type> lookup(Symbol symbol) {
-        Map<String, Type> table = tableByNamespace.get(symbol.getNamespace());
-        Type type = null;
+        Map<String, TypeSupplier> 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<Type> lookup(Symbol symbol) {
      * @return          symbols starting with the prefix
      */
     public Map<String, Type> lookupByPrefix(Symbol prefix) {
-        NavigableMap<String, Type> table = tableByNamespace.get(prefix.getNamespace());
+        NavigableMap<String, TypeSupplier> 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<String, Type> lookupByPrefix(Symbol prefix) {
      * @return              all symbols in the namespace map
      */
     public Map<String, Type> 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()));
     }
 
     /**

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<Type>} 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<Type> {
+    private final String symbolName;
+    private final Type symbolType;
+    private final Set<Type> 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;
+        }
+    }
+}

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<FieldMappings> 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<FieldMappings> 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<FieldMappings> 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<FieldMappings> 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() {

src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/semantic/visitor/SemanticAnalyzer.java

@@ -61,6 +61,12 @@ public Type visitSelect(List<Type> 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);

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<Type> 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();
     }

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());

src/main/java/com/amazon/opendistroforelasticsearch/sql/antlr/visitor/GenericSqlParseTreeVisitor.java

@@ -32,6 +32,10 @@ default T visitSelect(List<T> items) {
         return defaultValue();
     }
 
+    default T visitSelectAllColumn() {
+        return defaultValue();
+    }
+
     default void visitAs(String alias, T type) {}
 
     default T visitIndexName(String indexName) {

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<String, Object> 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<Map<String, Object>> optionalMap = curScope().resolveFieldMapping(expr.getName());
+                if (optionalMap.isPresent()) {
+                    source = optionalMap.get();
                 } else {
                     return true;
                 }
@@ -253,51 +251,6 @@ private void checkMappingCompatibility(TermFieldScope scope, Map<String, String>
         if (scope.getMapper().isEmpty()) {
             throw new VerificationException("Unknown index " + indexToType.keySet());
         }
-
-        Set<FieldMappings> indexMappings = curScope().getMapper().allMappings().stream().
-                flatMap(typeMappings -> typeMappings.allMappings().stream()).
-                collect(Collectors.toSet());
-
-        final FieldMappings fieldMappings;
-
-        if (indexMappings.size() > 1) {
-            Map<String, Map<String, Object>> mergedMapping = new HashMap<>();
-
-            for (FieldMappings f : indexMappings) {
-                Map<String, Map<String, Object>> 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<String, Object> fieldMapping,
-                                          final Map<String, Map<String, Object>> mergedMapping) {
-
-        if (!mergedMapping.containsKey(fieldName)) {
-            mergedMapping.put(fieldName, fieldMapping);
-        } else {
-
-            final Map<String, Object> 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<String, String> indexToType) {