Fix #46 #71
Fix #46 #71
Conversation
… test, add a new one
| @@ -210,12 +210,8 @@ public boolean isValidIdentifierForTerm(SQLIdentifierExpr expr) { | |||
| ); | |||
| } | |||
|
|
|||
| public boolean hasIdenticalMappings(TermFieldScope scope, Map<String, String> indexToType) { | |||
| public boolean hasUnknownIndex(TermFieldScope scope) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -102,6 +102,7 @@ public void testNonResolvingIndexPatternWithNonExistingIndex() throws IOExceptio | |||
| } | |||
|
|
|||
| @Test | |||
| @Ignore("allow non-identical mapping when query multi-index") | |||
There was a problem hiding this comment.
I prefer deleting the tests that are not asserting any truth anymore.
… check; delete useless test case
| @@ -210,19 +210,15 @@ public boolean isValidIdentifierForTerm(SQLIdentifierExpr expr) { | |||
| ); | |||
| } | |||
|
|
|||
| public boolean hasIdenticalMappings(TermFieldScope scope, Map<String, String> indexToType) { | |||
There was a problem hiding this comment.
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.
|
Switch to another approach. |
Issue #46 :
Testing: Passed gradle build
Description of changes:
Made minimal change to get rid of identical mapping check when query from multi-index, but still keeping the unknown index check. Updated corresponding tests.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.