[fix opendistro-for-elasticsearch#46] modify code to support multi-in…

…dices query with compatible mappings. Add new test cases and data. Delete useless test case
zhongnansu · Jun 14, 2019 · e5c46ef · e5c46ef
1 parent d0b51bd
commit e5c46ef
Show file tree

Hide file tree

Showing 7 changed files with 89 additions and 32 deletions.
diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/esdomain/LocalClusterState.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/esdomain/LocalClusterState.java
@@ -35,11 +35,7 @@
 import org.json.JSONObject;
 
 import java.io.IOException;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
+import java.util.*;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutionException;
 import java.util.function.Function;
@@ -399,6 +395,12 @@ public FieldMappings(MappingMetaData mappings) {
             fieldMappings = mappings.sourceAsMap();
         }
 
+        public FieldMappings(Map<String, Map<String, Object>> map) {
+            Map<String, Object> finalMap = new LinkedHashMap<>();
+            finalMap.put(PROPERTIES, map);
+            fieldMappings = finalMap;
+        }
+
         @Override
         public boolean has(String path) {
             return mapping(path) != null;

diff --git a/...ava/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldRewriter.java b/...ava/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldRewriter.java
@@ -35,6 +35,8 @@
 import java.util.Deque;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.LinkedHashMap;
+import java.util.Set;
 import java.util.stream.Collectors;
 
 import static com.amazon.opendistroforelasticsearch.sql.esdomain.LocalClusterState.FieldMappings;
@@ -72,9 +74,10 @@ public boolean visit(MySqlSelectQueryBlock query) {
         Map<String, String> indexToType = new HashMap<>();
         collect(query.getFrom(), indexToType, curScope().getAliases());
         curScope().setMapper(getMappings(indexToType));
+
         if ((this.filterType == TermRewriterFilter.COMMA || this.filterType == TermRewriterFilter.MULTI_QUERY)
-            && !hasIdenticalMappings(curScope(), indexToType)) {
-            throw new VerificationException("When using multiple indices, the mappings must be identical.");
+            && !hasCompatibleMappings(curScope(), indexToType)) {
+            throw new VerificationException("Mappings of indices are not compatible");
         }
         return true;
     }
@@ -210,18 +213,50 @@ public boolean isValidIdentifierForTerm(SQLIdentifierExpr expr) {
             );
     }
 
-    public boolean hasIdenticalMappings(TermFieldScope scope, Map<String, String> indexToType) {
+    public boolean hasCompatibleMappings(TermFieldScope scope, Map<String, String> indexToType) {
         if (scope.getMapper().isEmpty()) {
             throw new VerificationException("Unknown index " + indexToType.keySet());
         }
-
-        if (isMappingOfAllIndicesDifferent()) {
-            return false;
+        // Collect all FieldMappings into hash set and ignore index/type names. Size > 1 means FieldMappings NOT unique,
+        // needs further compatibility check
+        Set<FieldMappings> set = curScope().getMapper().allMappings().stream().
+                flatMap(typeMappings -> typeMappings.allMappings().stream()).
+                collect(Collectors.toSet());
+
+        if (set.size() > 1) {
+            Map<String, Map<String, Object>> map = new LinkedHashMap<>();
+            // Traverse each mapping
+            for (FieldMappings f : set) {
+                Map<String, Map<String, Object>> m = f.data();
+
+                for (Map.Entry<String, Map<String, Object>> e : m.entrySet()) {
+                    String key = e.getKey();
+                    Map<String, Object> columnMap = e.getValue();
+
+                    if (!map.containsKey(key)) {
+                        map.put(key, columnMap);
+                    } else {
+                        // check if types are same
+                        if (!columnMap.equals(map.get(key))) {
+                            /*todo: check if one has keyword, handle that case, need more consideration on what to store
+                               into the final mapping, the mapping used for select is different from condition query, with
+                               keyword involved*/
+
+                            // todo: throw exception with messages indicating the ambiguity from which field and which index
+                            return false;
+                        }
+                    }
+                }
+            }
+            // We need finalMapping to lookup for rewriting
+            FieldMappings fieldMappings = new FieldMappings(map);
+            curScope().setFinalMapping(fieldMappings);
+        } else {
+            // Indices have identical mapping
+            FieldMappings fieldMappings = curScope().getMapper().firstMapping().firstMapping();
+            curScope().setFinalMapping(fieldMappings);
         }
 
-        // We need finalMapping to lookup for rewriting
-        FieldMappings fieldMappings = curScope().getMapper().firstMapping().firstMapping();
-        curScope().setFinalMapping(fieldMappings);
         return true;
     }
 
@@ -246,12 +281,4 @@ public static String toString(TermRewriterFilter filter) {
             return filter.name;
         }
     }
-
-    private boolean isMappingOfAllIndicesDifferent() {
-        // Collect all FieldMappings into hash set and ignore index/type names. Size > 1 means FieldMappings NOT unique.
-        return curScope().getMapper().allMappings().stream().
-                                                    flatMap(typeMappings -> typeMappings.allMappings().stream()).
-                                                    collect(Collectors.toSet()).
-                                                    size() > 1;
-    }
 }
diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/SQLIntegTestCase.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/SQLIntegTestCase.java
@@ -216,6 +216,10 @@ public enum Index {
                 "dog",
                 TestUtils.getDogs2IndexMapping(),
                 "src/test/resources/dogs2.json"),
+        DOGS3(TestsConstants.TEST_INDEX_DOG3,
+                "dog",
+                TestUtils.getDogs3IndexMapping(),
+                "src/test/resources/dogs3.json"),
         PEOPLE(TestsConstants.TEST_INDEX_PEOPLE,
                 "people",
                 null,

diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TermQueryExplainIT.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TermQueryExplainIT.java
@@ -24,11 +24,7 @@
 import java.io.IOException;
 import java.util.Locale;
 
-import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_ACCOUNT;
-import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_BANK;
-import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_BANK_TWO;
-import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_DOG;
-import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_ONLINE;
+import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.*;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.not;
@@ -43,6 +39,8 @@ protected void init() throws Exception {
         loadIndex(Index.BANK);
         loadIndex(Index.BANK_TWO);
         loadIndex(Index.DOG);
+        loadIndex(Index.DOGS2);
+        loadIndex(Index.DOGS3);
     }
 
     @Test
@@ -102,19 +100,26 @@ public void testNonResolvingIndexPatternWithNonExistingIndex() throws IOExceptio
     }
 
     @Test
-    public void testNonIdenticalMappings() throws IOException {
+    public void testNonCompatibleMappings() throws IOException {
         try {
-            explainQuery(String.format(Locale.ROOT, "SELECT firstname, birthdate FROM %s, %s " +
-                            "WHERE firstname = 'Leo' OR male = 'true'",
-                    TEST_INDEX_BANK, TEST_INDEX_ONLINE));
+            explainQuery(String.format(Locale.ROOT, "SELECT * FROM %s, %s ",
+                    TEST_INDEX_DOG, TEST_INDEX_DOG2));
         } catch (ResponseException e) {
             assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(RestStatus.BAD_REQUEST.getStatus()));
             final String entity = TestUtils.getResponseBody(e.getResponse());
-            assertThat(entity, containsString("When using multiple indices, the mappings must be identical"));
+            assertThat(entity, containsString("Mappings of indices are not compatible"));
             assertThat(entity, containsString("\"type\": \"VerificationException\""));
         }
     }
 
+    @Test
+    public void testCompatibleMappings() throws IOException {
+        String result = explainQuery(String.format(Locale.ROOT, "SELECT color FROM %s, %s ",
+                    TEST_INDEX_DOG2, TEST_INDEX_DOG3));
+        assertThat(result, containsString("color"));
+        assertThat(result, containsString("_source"));
+    }
+
     @Test
     public void testIdenticalMappings() throws IOException {
 

diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TestUtils.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TestUtils.java
@@ -118,6 +118,20 @@ public static String getDogs2IndexMapping() {
                 "}";
     }
 
+    public static String getDogs3IndexMapping() {
+        return "{  \"dog\": {" +
+                " \"properties\": {\n" +
+                "          \"holdersName\": {\n" +
+                "            \"type\": \"keyword\"\n" +
+                "          },\n"+
+                "          \"color\": {\n" +
+                "            \"type\": \"text\"\n" +
+                "          }"+
+                "       }"+
+                "   }" +
+                "}";
+    }
+
     public static String getPeople2IndexMapping() {
         return "{  \"people\": {" +
                 " \"properties\": {\n" +

diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TestsConstants.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TestsConstants.java
@@ -28,6 +28,7 @@ public class TestsConstants {
     public final static String TEST_INDEX_PHRASE = TEST_INDEX + "_phrase";
     public final static String TEST_INDEX_DOG = TEST_INDEX + "_dog";
     public final static String TEST_INDEX_DOG2 = TEST_INDEX + "_dog2";
+    public final static String TEST_INDEX_DOG3 = TEST_INDEX + "_dog3";
     public final static String TEST_INDEX_PEOPLE = TEST_INDEX + "_people";
     public final static String TEST_INDEX_PEOPLE2 = TEST_INDEX + "_people2";
     public final static String TEST_INDEX_GAME_OF_THRONES = TEST_INDEX + "_game_of_thrones";

diff --git a/src/test/resources/dogs3.json b/src/test/resources/dogs3.json
@@ -0,0 +1,4 @@
+{"index":{"_type": "dog", "_id":"1"}}
+{"dog_name":"nelly","holdersName":"Bill","age":2, "color": "grey"}
+{"index":{"_type": "dog", "_id":"6"}}
+{"dog_name":"snoopy","holdersName":"Hattie","age":4, "color": "white"}