Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Fix #46 #71

Closed
wants to merge 5 commits into from
Closed

Fix #46 #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ecb779c
[fix #46] get rid of identical mapping check
zhongnansu Jun 5, 2019
7041361
Merge remote-tracking branch 'origin/master' into sql-46
zhongnansu Jun 5, 2019
d3b390d
[fix #46] ignore corresponding useless IT test, add a new one
zhongnansu Jun 5, 2019
e10f125
[fix #46] update new test method signature
zhongnansu Jun 5, 2019
0042559
[#46] fix semantic logic of unknown index check; delete useless test …
zhongnansu Jun 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 5 additions & 17 deletions ...ava/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldRewriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ public boolean visit(MySqlSelectQueryBlock query) {
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.");
&& hasUnknownIndex(curScope())) {
throw new VerificationException("Unknown index " + indexToType.keySet());
}
return true;
}
Expand Down Expand Up @@ -210,19 +210,15 @@ public boolean isValidIdentifierForTerm(SQLIdentifierExpr expr) {
);
}

public boolean hasIdenticalMappings(TermFieldScope scope, Map<String, String> indexToType) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that relaxing the mapping check completely will break the functionality where mappings are used to generate Elasticsearch DSL such as WHERE = clause, GROUP BY, ORDER By etc. This is because it will make it ambiguous as to mapping from which index to be applied. Lets keep this function, but only check for mappings present as part of SELECT clause.

public boolean hasUnknownIndex(TermFieldScope scope) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the definition of "unknown index"? Based on the code in this method, as well as on lines 76-77, I think the logic should be inverted.

Copy link
Member Author

@zhongnansu zhongnansu Jun 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. "unknown index" refers to index that is not existed, or can not be matched by wildcard. I modified the logic to make more sense.

if (scope.getMapper().isEmpty()) {
throw new VerificationException("Unknown index " + indexToType.keySet());
}

if (isMappingOfAllIndicesDifferent()) {
return false;
return true;
}

// We need finalMapping to lookup for rewriting
FieldMappings fieldMappings = curScope().getMapper().firstMapping().firstMapping();
curScope().setFinalMapping(fieldMappings);
return true;
return false;
}

public IndexMappings getMappings(Map<String, String> indexToType) {
Expand All @@ -246,12 +242,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;
}
}
19 changes: 8 additions & 11 deletions src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TermQueryExplainIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,17 +102,14 @@ public void testNonResolvingIndexPatternWithNonExistingIndex() throws IOExceptio
}

@Test
public void testNonIdenticalMappings() 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));
} 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("\"type\": \"VerificationException\""));
}
public void testAllowNonIdenticalMappings() throws IOException {
String result = explainQuery(String.format(Locale.ROOT, "SELECT firstname, birthdate FROM %s, %s " +
"WHERE firstname = 'Leo' OR male = 'true'",
TEST_INDEX_BANK, TEST_INDEX_ONLINE));
assertThat(result, containsString("term"));
assertThat(result, containsString("_source"));
assertThat(result, containsString("firstname"));
assertThat(result, containsString("birthdate"));
}

@Test
Expand Down