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 4 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
18 changes: 3 additions & 15 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,12 +210,8 @@ 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;
}

Expand Down Expand Up @@ -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;
}
}
12 changes: 12 additions & 0 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,6 +102,7 @@ public void testNonResolvingIndexPatternWithNonExistingIndex() throws IOExceptio
}

@Test
@Ignore("allow non-identical mapping when query multi-index")
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer deleting the tests that are not asserting any truth anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Deleted

public void testNonIdenticalMappings() throws IOException {
try {
explainQuery(String.format(Locale.ROOT, "SELECT firstname, birthdate FROM %s, %s " +
Expand All @@ -115,6 +116,17 @@ public void testNonIdenticalMappings() throws IOException {
}
}

@Test
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
public void testIdenticalMappings() throws IOException {

Expand Down