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

Support to parse backticks quoted identifiers #240

Merged
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
920830a
Added an unquoter utils class to extract string text from the back-ticks
Oct 21, 2019
3e3a91e
Modified the semantic analyzer to visit the index names and field nam…
Oct 21, 2019
5ef2ba8
Added an UnquoteIdentifierRule rewrite the AST that are constructed f…
Oct 21, 2019
a91696b
Moved the rule into "identifier" package under the rewriter
Oct 21, 2019
eef1203
Added unittest for the backticksUnquoter utils class
Oct 21, 2019
f851098
Added integtest for the unquote rule
Oct 21, 2019
b325d44
Modified the unquoter to make it reusable, and moved into StringUtils…
Oct 21, 2019
272f2c7
Modified unittest
Oct 21, 2019
614c78e
Added proper condition to modify the SQLSelectItem in visiting method
Oct 21, 2019
8a0c4c6
Added comments to explain the major methods in the rewriter rule.
Oct 21, 2019
5968924
Moved pretty formatter test under utils test class
Oct 21, 2019
d9796c8
Modified the unquoter IT
Oct 21, 2019
157ed37
Added UT for the updated ANTLR parser
Oct 21, 2019
76b1eb7
Used Strings.isNullOrEmpty()
Oct 21, 2019
9ca3688
Reused unquoteSingleField method; modified the substring generating code
Oct 21, 2019
8201a8d
Updated
Oct 21, 2019
bc876ef
Updated
Oct 21, 2019
97eeec8
Added IT to test the field alias in JDBC response; seperated backtick…
Oct 21, 2019
bf8445c
Updated intro of rewriter rule
Oct 21, 2019
aa29c0f
Added unittest for UnquoteIdentifierRule
Oct 22, 2019
c7b4216
Updated
Oct 22, 2019
4332813
Updated
Oct 22, 2019
d66f233
Updated
Oct 22, 2019
afa4684
Updated
Oct 22, 2019
44b4c1a
Updated
Oct 22, 2019
ba1235b
Added UT and IT for quoted <table>.<column> case
Oct 22, 2019
72f92de
Added one UT case
Oct 22, 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
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import com.amazon.opendistroforelasticsearch.sql.antlr.semantic.types.Type;
import com.amazon.opendistroforelasticsearch.sql.antlr.visitor.GenericSqlParseTreeVisitor;
import com.amazon.opendistroforelasticsearch.sql.utils.StringUtils;

import java.util.List;

Expand Down Expand Up @@ -60,20 +61,20 @@ public Type visitSelect(List<Type> itemTypes) {

@Override
public void visitAs(String alias, Type type) {
mappingLoader.visitAs(alias, type);
typeChecker.visitAs(alias, type);
mappingLoader.visitAs(StringUtils.unquoteSingleField(alias, "`"), type);
typeChecker.visitAs(StringUtils.unquoteSingleField(alias, "`"), type);
}

@Override
public Type visitIndexName(String indexName) {
mappingLoader.visitIndexName(indexName);
return typeChecker.visitIndexName(indexName);
mappingLoader.visitIndexName(StringUtils.unquoteSingleField(indexName, "`"));
return typeChecker.visitIndexName(StringUtils.unquoteSingleField(indexName, "`"));
}

@Override
public Type visitFieldName(String fieldName) {
mappingLoader.visitFieldName(fieldName);
return typeChecker.visitFieldName(fieldName);
mappingLoader.visitFieldName(StringUtils.unquoteFullColumn(fieldName, "`"));
return typeChecker.visitFieldName(StringUtils.unquoteFullColumn(fieldName, "`"));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import com.amazon.opendistroforelasticsearch.sql.query.multi.MultiQueryAction;
import com.amazon.opendistroforelasticsearch.sql.query.multi.MultiQuerySelect;
import com.amazon.opendistroforelasticsearch.sql.rewriter.RewriteRuleExecutor;
import com.amazon.opendistroforelasticsearch.sql.rewriter.identifier.UnquoteIdentifierRule;
import com.amazon.opendistroforelasticsearch.sql.rewriter.alias.TableAliasPrefixRemoveRule;
import com.amazon.opendistroforelasticsearch.sql.rewriter.matchtoterm.TermFieldRewriter;
import com.amazon.opendistroforelasticsearch.sql.rewriter.matchtoterm.TermFieldRewriter.TermRewriterFilter;
Expand Down Expand Up @@ -78,6 +79,7 @@ public static QueryAction create(Client client, String sql) throws SqlParseExcep

RewriteRuleExecutor<SQLQueryExpr> ruleExecutor = RewriteRuleExecutor.builder()
.withRule(new SQLExprParentSetterRule())
.withRule(new UnquoteIdentifierRule())
.withRule(new TableAliasPrefixRemoveRule())
.withRule(new SubQueryRewriteRule())
.build();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
/*
* Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

package com.amazon.opendistroforelasticsearch.sql.rewriter.identifier;

import com.alibaba.druid.sql.ast.expr.SQLIdentifierExpr;
import com.alibaba.druid.sql.ast.expr.SQLPropertyExpr;
import com.alibaba.druid.sql.ast.expr.SQLQueryExpr;
import com.alibaba.druid.sql.ast.statement.SQLExprTableSource;
import com.alibaba.druid.sql.ast.statement.SQLSelectItem;
import com.alibaba.druid.sql.dialect.mysql.visitor.MySqlASTVisitorAdapter;
import com.amazon.opendistroforelasticsearch.sql.rewriter.RewriteRule;
import com.amazon.opendistroforelasticsearch.sql.utils.StringUtils;

public class UnquoteIdentifierRule extends MySqlASTVisitorAdapter implements RewriteRule<SQLQueryExpr> {
chloe-zh marked this conversation as resolved.
Show resolved Hide resolved

private String identifier = null;
Copy link
Member

Choose a reason for hiding this comment

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

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.


/**
*
* This method is to adjust the AST in the cases where the field is quoted,
* and the full name in the SELECT field is in the format of indexAlias.fieldName
* (e.g. SE:ECT b.`lastname` FROM bank AS b).
chloe-zh marked this conversation as resolved.
Show resolved Hide resolved
*
* In this case, the druid parser constructs a SQLSelectItem for the field "b.`lastname`", with SQLIdentifierExpr of
* "b." and alias of "`lastname`".
*
* This method corrects the SQLSelectItem object to have SQLIdentifier of "b.lastname" and alias of null.
*/
@Override
public boolean visit(SQLSelectItem selectItem) {
if (selectItem.getExpr() instanceof SQLIdentifierExpr) {
String identifier = ((SQLIdentifierExpr) selectItem.getExpr()).getName();
chloe-zh marked this conversation as resolved.
Show resolved Hide resolved
if (identifier.endsWith(".")) {
this.identifier = identifier + StringUtils.unquoteSingleField(selectItem.getAlias(), "`");
selectItem.setExpr(new SQLIdentifierExpr(this.identifier));
selectItem.setAlias(null);
}
}
return true;
}

/**
*
* This method is to adjust the AST in the cases where the alias of index is quoted
* (e.g. SELECT `b`.lastname FROM bank AS `b`).
*
* In this case, the druid parser constructs a SQLPropertyExpr for the field "`b`.lastname", with owner of a
* SQLIdentifierExpr "`b`" and name of "lastname".
*
* This method prevent the visitor from visitin the SQLPropertyExpr in this case,
* and corrects AST with a SQLSelectItem object to have SQLIdentifier of "b.lastname".
*
* Used in the case where alias of index and the field name are both quoted
* (e.g. SELECT `b`.`lastname` FROM bank AS `b`).
*/
@Override
public boolean visit(SQLPropertyExpr propertyExpr) {
penghuo marked this conversation as resolved.
Show resolved Hide resolved
String fieldName = ((SQLIdentifierExpr) propertyExpr.getOwner()).getName();
if (!StringUtils.isQuoted(fieldName, "`")) {
return true;
}
this.identifier = StringUtils.unquoteSingleField(fieldName, "`") + "."
+ StringUtils.unquoteSingleField(propertyExpr.getName(), "`");
SQLSelectItem selectItem = (SQLSelectItem) propertyExpr.getParent();
selectItem.setExpr(new SQLIdentifierExpr(this.identifier));
this.identifier = null;
return false;
}

@Override
public boolean visit(SQLIdentifierExpr identifierExpr) {
if (identifier != null) {
return false;
}
return true;
}

@Override
public void endVisit(SQLIdentifierExpr identifierExpr) {
if (identifier != null) {
chloe-zh marked this conversation as resolved.
Show resolved Hide resolved
identifierExpr.setName(identifier);
identifier = null;
} else {
identifierExpr.setName(StringUtils.unquoteFullColumn(identifierExpr.getName(), "`"));
}
}

@Override
public void endVisit(SQLExprTableSource tableSource) {
tableSource.setAlias(StringUtils.unquoteSingleField(tableSource.getAlias(), "`"));
}

@Override
public boolean match(SQLQueryExpr root) {
chloe-zh marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

@Override
public void rewrite(SQLQueryExpr root) {
chloe-zh marked this conversation as resolved.
Show resolved Hide resolved
root.accept(new UnquoteIdentifierRule());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

package com.amazon.opendistroforelasticsearch.sql.utils;

import jdk.internal.joptsimple.internal.Strings;

import java.util.Locale;

/**
Expand Down Expand Up @@ -80,6 +82,43 @@ public static int countMatches(CharSequence input, char match) {
count());
}

/**
*
* @param text string
* @param quote
* @return An unquoted string whose outer pair of back-ticks (if any) has been removed
*/
public static String unquoteSingleField(String text, String quote) {
if (isQuoted(text, quote)) {
return text.substring(quote.length(), text.length() - quote.length());
}
return text;
}

/**
*
* @param text
* @return A string whose each dot-seperated field has been unquoted from back-ticks (if any)
*/
public static String unquoteFullColumn(String text, String quote) {
if (Strings.isNullOrEmpty(text)) {
return null;
}
String[] strs = text.split("\\.");
for (int i = 0; i < strs.length; i++) {
String unquotedSubstr = unquoteSingleField(strs[i], quote);
strs[i] = unquotedSubstr;
}
return String.join(".", strs);
}

public static boolean isQuoted(String text, String quote) {
if (!Strings.isNullOrEmpty(text) && text.startsWith(quote) && text.endsWith(quote)) {
return true;
}
return false;
}

private StringUtils() {
throw new AssertionError(getClass().getCanonicalName() + " is a utility class and must not be initialized");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,4 +155,21 @@ public void useConstantLiteralInSelectClauseShouldPass() {
validate("SELECT TRUE FROM semantics");
}

@Test
public void queryWithBackticksQuotedIndexShouldPass() {
validate("SELECT age FROM `semantics`");
}

@Test
public void queryWithBackticksQuotedIndexAliasShouldPass() {
validate("SELECT `s`.age FROM semantics AS `s`");
validate("SELECT `s t`.age FROM semantics AS `s t`");
}

@Test
public void queryWithBackticksQuotedFieldNameShouldPass() {
validate("SELECT `age` FROM semantics");
validate("SELECT s.`age` FROM semantics AS s");
validate("SELECT `s`.`age` FROM semantics AS `s`");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1671,6 +1671,41 @@ public void fieldCollapsingTest() throws IOException {
Assert.assertEquals(21, hits.length());
}

@Test
public void backticksQuotedIndexNameTest() throws Exception {
AdminClient adminClient = this.admin();
TestUtils.createTestIndex(adminClient, "bank_unquote", "bank_unquote", null);
TestUtils.loadBulk(ESIntegTestCase.client(),
"/src/test/resources/bank_for_unquote_test.json", "bank");

final String query = "SELECT lastname FROM `bank` ORDER BY age LIMIT 3";
JSONObject response = executeQuery(query);
JSONArray hits = getHits(response);

assertThat("bank", equalTo(((JSONObject) hits.get(0)).query("/_index")));
}

@Test
public void backticksQuotedFieldNamesTest() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we test so many queries within one method?

dai-chen marked this conversation as resolved.
Show resolved Hide resolved
final String basicQuery = StringUtils.format("SELECT b.lastname FROM %s " +
"AS b ORDER BY age LIMIT 3", TestsConstants.TEST_INDEX_BANK);
final String queryWithQuotedAlias = StringUtils.format("SELECT `b`.lastname FROM %s" +
" AS `b` ORDER BY age LIMIT 3", TestsConstants.TEST_INDEX_BANK);
final String queryWithQuotedFieldName = StringUtils.format("SELECT b.`lastname` FROM %s " +
"AS b ORDER BY age LIMIT 3", TestsConstants.TEST_INDEX_BANK);
final String queryWithQuotedAliasAndFieldName = StringUtils.format("SELECT `b`.`lastname` FROM %s " +
"AS `b` ORDER BY age LIMIT 3", TestsConstants.TEST_INDEX_BANK);
final String queryWithQuotedAliasContainingSpecialChars = StringUtils.format("SELECT `b k`.lastname FROM %s " +
"AS `b k` ORDER BY age LIMIT 3", TestsConstants.TEST_INDEX_BANK);

final String expectedResponse = executeQuery(basicQuery, "jdbc");

assertThat(expectedResponse, equalTo(executeQuery(queryWithQuotedAlias, "jdbc")));
assertThat(expectedResponse, equalTo(executeQuery(queryWithQuotedFieldName, "jdbc")));
assertThat(expectedResponse, equalTo(executeQuery(queryWithQuotedAliasAndFieldName, "jdbc")));
assertThat(expectedResponse, equalTo(executeQuery(queryWithQuotedAliasContainingSpecialChars, "jdbc")));
}

private String getScrollId(JSONObject response) {
return response.getString("_scroll_id");
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*
* Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

package com.amazon.opendistroforelasticsearch.sql.unittest.utils;

import com.amazon.opendistroforelasticsearch.sql.utils.StringUtils;
import org.junit.Test;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.IsEqual.equalTo;

/**
* To test the functionality of {@link StringUtils#unquoteSingleField}
* and {@link StringUtils#unquoteFullColumn(String, String)}
*/
public class BackticksUnquoterTest {
dai-chen marked this conversation as resolved.
Show resolved Hide resolved

@Test
public void assertNotQuotedStringShouldKeepTheSame() {
String originalText = "identifier";
String resultForUnquotingSingleField = StringUtils.unquoteSingleField(originalText, "`");
String resultForUnquotingFullColumn = StringUtils.unquoteFullColumn(originalText, "`");

assertThat(resultForUnquotingSingleField, equalTo(originalText));
assertThat(resultForUnquotingFullColumn, equalTo(originalText));
}

@Test
public void assertStringWithOneBackTickShouldKeepTheSame() {
String originalText = "`identifier";
String result1 = StringUtils.unquoteSingleField(originalText, "`");
String result2 = StringUtils.unquoteFullColumn(originalText, "`");

assertThat(result1, equalTo(originalText));
assertThat(result2, equalTo(originalText));
}

@Test
public void assertBackticksQuotedStringShouldBeUnquoted() {
String originalText1 = "`identifier`";
String originalText2 = "`identifier1`.`identifier2`";
String originalText3 = "`identifier1`.identifier2";

String expectedResult1 = "identifier";
String expectedResult2 = "identifier1.identifier2";

String result1 = StringUtils.unquoteSingleField(originalText1, "`");
String result2 = StringUtils.unquoteFullColumn(originalText2, "`");
String result3 = StringUtils.unquoteFullColumn(originalText3, "`");

assertThat(expectedResult1, equalTo(result1));
assertThat(expectedResult2, equalTo(result2));
assertThat(expectedResult2, equalTo(result3));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* permissions and limitations under the License.
*/

package com.amazon.opendistroforelasticsearch.sql.unittest;
package com.amazon.opendistroforelasticsearch.sql.unittest.utils;

import com.amazon.opendistroforelasticsearch.sql.esintgtest.TestUtils;
import com.amazon.opendistroforelasticsearch.sql.utils.JsonPrettyFormatter;
Expand Down
14 changes: 14 additions & 0 deletions src/test/resources/bank_for_unquote_test.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{"index":{"_type": "account_unquote","_id":"1"}}
{"account_number":1,"balance":39225,"firstname":"Amber JOHnny","lastname":"Duke Willmington","age":32,"gender":"M","address":"880 Holmes Lane","employer":"Pyrami","email":"[email protected]","city":"Brogan","state":"IL", "male" : true, "birthdate" : "2017-10-23"}
{"index":{"_type": "account_unquote","_id":"6"}}
{"account_number":6,"balance":5686,"firstname":"Hattie","lastname":"Bond","age":36,"gender":"M","address":"671 Bristol Street","employer":"Netagy","email":"[email protected]","city":"Dante","state":"TN", "male" : true, "birthdate" : "2017-11-20"}
{"index":{"_type": "account_unquote","_id":"13"}}
{"account_number":13,"balance":32838,"firstname":"Nanette","lastname":"Bates","age":28,"gender":"F","address":"789 Madison Street","employer":"Quility","email":"[email protected]","city":"Nogal","state":"VA", "male" : false, "birthdate" : "2018-06-23"}
{"index":{"_type": "account_unquote","_id":"18"}}
{"account_number":18,"balance":4180,"firstname":"Dale","lastname":"Adams","age":33,"gender":"M","address":"467 Hutchinson Court","employer":"Boink","email":"[email protected]","city":"Orick","state":"MD","male" : true, "birthdate" : "1542152000"}
{"index":{"_type": "account_unquote","_id":"20"}}
{"account_number":20,"balance":16418,"firstname":"Elinor","lastname":"Ratliff","age":36,"gender":"M","address":"282 Kings Place","employer":"Scentric","email":"[email protected]","city":"Ribera","state":"WA", "male" : true, "birthdate" : "2018-06-27"}
{"index":{"_type": "account_unquote","_id":"25"}}
{"account_number":25,"balance":40540,"firstname":"Virginia","lastname":"Ayala","age":39,"gender":"F","address":"171 Putnam Avenue","employer":"Filodyne","email":"[email protected]","city":"Nicholson","state":"PA", "male" : false, "birthdate" : "2018-08-19"}
{"index":{"_type": "account_unquote","_id":"32"}}
{"account_number":32,"balance":48086,"firstname":"Dillard","lastname":"Mcpherson","age":34,"gender":"F","address":"702 Quentin Street","employer":"Quailcom","email":"[email protected]","city":"Veguita","state":"IN", "male" : false, "birthdate" : "2018-08-11"}