Support to parse backticks quoted identifiers #240

chloe-zh · 2019-10-21T16:08:30Z

Issue #, if available:

Issue #212 Tableau: Quoted identifiers not supported

Description of changes:

Modified the semantic analyzer to unquote the index names and field names for the AST visitor, so that the identifiers would pass the new parser correctly.
In the cases with back-ticks quoted identifiers, the AST is constructed improperly, so in this PR a rewriter rule is added to adjust the AST case by case.
Added UT and IT.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…es that have been unquoted from the back-ticks

…rom the druid parser

...ava/com/amazon/opendistroforelasticsearch/sql/rewriter/identifier/UnquoteIdentifierRule.java

davidcui1225 · 2019-10-21T17:03:29Z

src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/QueryIT.java

+    }
+
+    @Test
+    public void backticksQuotedFieldNamesTest() {


Should we test so many queries within one method?

src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/BackticksUnquoter.java

...est/java/com/amazon/opendistroforelasticsearch/sql/unittest/utils/BackticksUnquoterTest.java

...ava/com/amazon/opendistroforelasticsearch/sql/rewriter/identifier/UnquoteIdentifierRule.java

src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/BackticksUnquoter.java

galkk · 2019-10-21T17:29:16Z

src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/BackticksUnquoter.java

+        return unquotedStrBuilder.toString();
+    }
+
+    public BackticksUnquoter() {


you don't need to instantiate this class, make constructor private

...est/java/com/amazon/opendistroforelasticsearch/sql/unittest/utils/BackticksUnquoterTest.java

… class, made corresponding changes for testing

...ava/com/amazon/opendistroforelasticsearch/sql/rewriter/identifier/UnquoteIdentifierRule.java

src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/StringUtils.java

arsenmk · 2019-10-21T20:52:15Z

src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/StringUtils.java

+    }
+
+    public static boolean isQuoted(String text, String quote) {
+        if (text != null && text.startsWith(quote) && text.endsWith(quote)) {


np: I would assume that if the text starts with a quote, then it has to end with a quote, otherwise the parser should have thrown exception before reaching to this method call.

Also, just return the if condition expression in one line.

dai-chen · 2019-10-21T21:27:36Z

...ava/com/amazon/opendistroforelasticsearch/sql/rewriter/identifier/UnquoteIdentifierRule.java

+
+public class UnquoteIdentifierRule extends MySqlASTVisitorAdapter implements RewriteRule<SQLQueryExpr> {
+
+    private String identifier = null;


Could you elaborate what is the use of this field variable? Because I feel this is risky to maintain global state between visit methods. Trying to understand its intent and see if other choice.

...ava/com/amazon/opendistroforelasticsearch/sql/rewriter/identifier/UnquoteIdentifierRule.java

src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/QueryIT.java

…s-quoted identifier ITs into index test, field test, alias test

davidcui1225 · 2019-10-22T16:07:45Z

src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/QueryIT.java

+    }
+
+    @Test
+    public void backticksQuotedAliasWithSpecialCharactersTest() {


Are there any other special characters that we should test against?

Good point. I was wondering as well if there are any other special characters that would be generated by Tableau. But I think to support the space in alias is enough based on the other chars (eg. "_" etc.) that have been supported to pass the parse so far.

dai-chen

LGTM. Could you do me a favor by adding one more case in your UT and IT for SELECT `bank`.`lastname` from `bank` ? This was the issue #213 I fixed earlier but unable to verify this case without your changes. Thanks!

chloe-zh · 2019-10-22T20:08:42Z

LGTM. Could you do me a favor by adding one more case in your UT and IT for SELECT `bank`.`lastname` from `bank`? This was the issue #213 I fixed earlier but unable to verify this case without your changes. Thanks!

@dai-chen Yes, sure. Thanks!

davidcui1225

LGTM

chloe-zh added 6 commits October 20, 2019 19:15

Added an unquoter utils class to extract string text from the back-ticks

920830a

Modified the semantic analyzer to visit the index names and field nam…

3e3a91e

…es that have been unquoted from the back-ticks

Added an UnquoteIdentifierRule rewrite the AST that are constructed f…

5ef2ba8

…rom the druid parser

Moved the rule into "identifier" package under the rewriter

a91696b

Added unittest for the backticksUnquoter utils class

eef1203

Added integtest for the unquote rule

f851098

chloe-zh requested review from galkk, penghuo, dai-chen, abbashus and davidcui1225 October 21, 2019 16:08

davidcui1225 reviewed Oct 21, 2019

View reviewed changes

dai-chen suggested changes Oct 21, 2019

View reviewed changes