From e5c46efbd754821d693b10ee101fc8e4f307788f Mon Sep 17 00:00:00 2001 From: zhongnansu Date: Thu, 13 Jun 2019 17:18:06 -0700 Subject: [PATCH] [fix #46] modify code to support multi-indices query with compatible mappings. Add new test cases and data. Delete useless test case --- .../sql/esdomain/LocalClusterState.java | 12 ++-- .../matchtoterm/TermFieldRewriter.java | 61 +++++++++++++------ .../sql/esintgtest/SQLIntegTestCase.java | 4 ++ .../sql/esintgtest/TermQueryExplainIT.java | 25 +++++--- .../sql/esintgtest/TestUtils.java | 14 +++++ .../sql/esintgtest/TestsConstants.java | 1 + src/test/resources/dogs3.json | 4 ++ 7 files changed, 89 insertions(+), 32 deletions(-) create mode 100644 src/test/resources/dogs3.json diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/esdomain/LocalClusterState.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/esdomain/LocalClusterState.java index 0bfb92f7c2..cbb88c74d1 100644 --- 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> map) { + Map finalMap = new LinkedHashMap<>(); + finalMap.put(PROPERTIES, map); + fieldMappings = finalMap; + } + @Override public boolean has(String path) { return mapping(path) != null; 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 5bd138392f..e917ea49c7 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,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 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 indexToType) { + public boolean hasCompatibleMappings(TermFieldScope scope, Map 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 set = curScope().getMapper().allMappings().stream(). + flatMap(typeMappings -> typeMappings.allMappings().stream()). + collect(Collectors.toSet()); + + if (set.size() > 1) { + Map> map = new LinkedHashMap<>(); + // Traverse each mapping + for (FieldMappings f : set) { + Map> m = f.data(); + + for (Map.Entry> e : m.entrySet()) { + String key = e.getKey(); + Map 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 index 086acd18c0..efc727a38b 100644 --- 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 index fa96fed6d8..987287cdb6 100644 --- 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 index f3fd815bd9..4669c488e8 100644 --- 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 index f437eaa528..76b3e13dbb 100644 --- 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 new file mode 100644 index 0000000000..07195836f0 --- /dev/null +++ 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"}