From b5922e51495ccd81094a13b824fe8ed60fe3ef88 Mon Sep 17 00:00:00 2001 From: Hussain Date: Wed, 23 Oct 2019 11:07:50 -0700 Subject: [PATCH 01/12] Support for ordinal alias in GROUP BY and ORDER BY clause --- .../sql/query/ESActionFactory.java | 5 +- .../rewriter/ordinal/OrdinalRewriterRule.java | 196 ++++++++++++++++++ 2 files changed, 199 insertions(+), 2 deletions(-) create mode 100644 src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/ESActionFactory.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/ESActionFactory.java index 733b0815b1..c591b19c7d 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/ESActionFactory.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/ESActionFactory.java @@ -45,6 +45,7 @@ import com.amazon.opendistroforelasticsearch.sql.rewriter.matchtoterm.TermFieldRewriter; import com.amazon.opendistroforelasticsearch.sql.rewriter.matchtoterm.TermFieldRewriter.TermRewriterFilter; import com.amazon.opendistroforelasticsearch.sql.rewriter.nestedfield.NestedFieldRewriter; +import com.amazon.opendistroforelasticsearch.sql.rewriter.ordinal.OrdinalRewriterRule; import com.amazon.opendistroforelasticsearch.sql.rewriter.parent.SQLExprParentSetterRule; import com.amazon.opendistroforelasticsearch.sql.rewriter.subquery.SubQueryRewriteRule; import org.elasticsearch.client.Client; @@ -80,6 +81,7 @@ public static QueryAction create(Client client, String sql) throws SqlParseExcep .withRule(new SQLExprParentSetterRule()) .withRule(new TableAliasPrefixRemoveRule()) .withRule(new SubQueryRewriteRule()) + .withRule(new OrdinalRewriterRule(sql)) .build(); ruleExecutor.executeOn(sqlExpr); sqlExpr.accept(new NestedFieldRewriter()); @@ -173,9 +175,8 @@ private static SQLExpr toSqlExpr(String sql) { SQLExpr expr = parser.expr(); if (parser.getLexer().token() != Token.EOF) { - throw new ParserException("illegal sql expr : " + sql); + throw new ParserException("Illegal SQL expression : " + sql); } - return expr; } } diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java new file mode 100644 index 0000000000..be25280c17 --- /dev/null +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java @@ -0,0 +1,196 @@ +/* + * 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.ordinal; + +import com.alibaba.druid.sql.ast.SQLExpr; +import com.alibaba.druid.sql.ast.expr.SQLBinaryOpExpr; +import com.alibaba.druid.sql.ast.expr.SQLIntegerExpr; +import com.alibaba.druid.sql.ast.expr.SQLQueryExpr; +import com.alibaba.druid.sql.ast.statement.SQLSelectItem; +import com.alibaba.druid.sql.ast.statement.SQLSelectOrderByItem; +import com.alibaba.druid.sql.ast.statement.SQLSelectQuery; +import com.alibaba.druid.sql.dialect.mysql.ast.expr.MySqlSelectGroupByExpr; +import com.alibaba.druid.sql.dialect.mysql.ast.statement.MySqlSelectQueryBlock; +import com.alibaba.druid.sql.dialect.mysql.visitor.MySqlASTVisitorAdapter; +import com.alibaba.druid.sql.parser.ParserException; +import com.alibaba.druid.sql.parser.SQLExprParser; +import com.alibaba.druid.sql.parser.Token; +import com.amazon.opendistroforelasticsearch.sql.parser.ElasticSqlExprParser; +import com.amazon.opendistroforelasticsearch.sql.rewriter.RewriteRule; +import com.amazon.opendistroforelasticsearch.sql.rewriter.RewriteRuleExecutor; +import com.amazon.opendistroforelasticsearch.sql.rewriter.alias.TableAliasPrefixRemoveRule; + +import java.sql.SQLFeatureNotSupportedException; +import java.util.HashMap; +import java.util.Map; + +/** + * Rewrite rule for changing ordinal alias in order by and group by to actual select field. + */ +public class OrdinalRewriterRule implements RewriteRule { + + private SQLQueryExpr sqlExprGroupCopy; + private SQLQueryExpr sqlExprOrderCopy; + private final String sql; + private final Map groupOrdinalToExpr = new HashMap<>(); + private final Map orderOrdinalToExpr = new HashMap<>(); + private RewriteRuleExecutor ruleExecutor; + + public OrdinalRewriterRule(String sql) { + this.sql = sql; + } + + @Override + public boolean match(SQLQueryExpr root) throws SQLFeatureNotSupportedException { + SQLSelectQuery sqlSelectQuery = root.getSubQuery().getQuery(); + if (!(sqlSelectQuery instanceof MySqlSelectQueryBlock)) { + // it could be SQLUnionQuery + return false; + } + + MySqlSelectQueryBlock query = (MySqlSelectQueryBlock) sqlSelectQuery; + + if (!hasGroupBy(query) && !hasOrderBy(query)) { + return false; + } + + // we cannot clone SQLSelectItem, so we need similar objects to assign to GroupBy and OrderBy items + sqlExprGroupCopy = generateAST(); + sqlExprOrderCopy = generateAST(); + + collectOrdinalAliases(sqlExprGroupCopy, groupOrdinalToExpr); + collectOrdinalAliases(sqlExprOrderCopy, orderOrdinalToExpr); + + if (groupOrdinalToExpr.isEmpty()) { + // orderOrdinalToExpr and groupOrdinalToExpr should be identical + // because they are generated from similar syntax trees + // checking for groupOrdinalToExpr or orderOrdinalToExpr is enough + return false; + } + + // we need to rewrite the generated syntax tree to match the original syntax tree Select clause + ruleExecutor = RewriteRuleExecutor.builder() + .withRule(new TableAliasPrefixRemoveRule()) + .build(); + + ruleExecutor.executeOn(sqlExprGroupCopy); + ruleExecutor.executeOn(sqlExprGroupCopy); + return true; + } + + @Override + public void rewrite(SQLQueryExpr root) { + changeOrdinalAliasInGroupAndOrderBy(root); + } + + private boolean hasGroupBy(MySqlSelectQueryBlock query) { + if (query.getGroupBy() == null) { + return false; + } else if (query.getGroupBy().getItems().isEmpty()){ + return false; + } + + return query.getGroupBy().getItems().stream().anyMatch(x -> + x instanceof MySqlSelectGroupByExpr && ((MySqlSelectGroupByExpr) x).getExpr() instanceof SQLIntegerExpr + ); + } + + private boolean hasOrderBy(MySqlSelectQueryBlock query) { + if (query.getOrderBy() == null) { + return false; + } else if (query.getOrderBy().getItems().isEmpty()){ + return false; + } + + return query.getOrderBy().getItems().stream().anyMatch(x -> + x.getExpr() instanceof SQLIntegerExpr + || ( + x.getExpr() instanceof SQLBinaryOpExpr + && ((SQLBinaryOpExpr) x.getExpr()).getLeft() instanceof SQLIntegerExpr + ) + ); + } + + private void collectOrdinalAliases(SQLQueryExpr root, Map map) { + root.accept(new MySqlASTVisitorAdapter() { + private Integer count = 0; + + @Override + public boolean visit(SQLSelectItem select) { + map.put(++count, select.getExpr()); + return false; + } + }); + } + + private void changeOrdinalAliasInGroupAndOrderBy(SQLQueryExpr root) { + root.accept(new MySqlASTVisitorAdapter() { + + @Override + public boolean visit(MySqlSelectGroupByExpr groupByExpr) { + SQLExpr expr = groupByExpr.getExpr(); + if (expr instanceof SQLIntegerExpr) { + Integer ordinalValue = ((SQLIntegerExpr) expr).getNumber().intValue(); + SQLExpr newExpr = groupOrdinalToExpr.get(ordinalValue); + groupByExpr.setExpr(newExpr); + newExpr.setParent(groupByExpr); + } + return false; + } + + @Override + public boolean visit(SQLSelectOrderByItem orderByItem) { + SQLExpr expr = orderByItem.getExpr(); + Integer ordinalValue; + + if (expr instanceof SQLIntegerExpr) { + ordinalValue = ((SQLIntegerExpr) expr).getNumber().intValue(); + SQLExpr newExpr = orderOrdinalToExpr.get(ordinalValue); + + orderByItem.setExpr(newExpr); + newExpr.setParent(orderByItem); + }else if (expr instanceof SQLBinaryOpExpr + && ((SQLBinaryOpExpr) expr).getLeft() instanceof SQLIntegerExpr) { + // support ORDER BY IS NULL/NOT NULL + SQLBinaryOpExpr binaryOpExpr = (SQLBinaryOpExpr) expr; + SQLIntegerExpr integerExpr = (SQLIntegerExpr) binaryOpExpr.getLeft(); + + ordinalValue = integerExpr.getNumber().intValue(); + SQLExpr newExpr = orderOrdinalToExpr.get(ordinalValue); + binaryOpExpr.setLeft(newExpr); + newExpr.setParent(binaryOpExpr); + } + + return false; + } + }); + } + + private SQLQueryExpr generateAST() { + return (SQLQueryExpr) toSqlExpr(sql); + } + + private static SQLExpr toSqlExpr(String sql) { + SQLExprParser parser = new ElasticSqlExprParser(sql); + SQLExpr expr = parser.expr(); + + if (parser.getLexer().token() != Token.EOF) { + throw new ParserException("Illegal SQL expression : " + sql); + } + return expr; + } +} From 69edbf43b9f15b45826182130199040f5c9a2356 Mon Sep 17 00:00:00 2001 From: Hussain Date: Wed, 23 Oct 2019 11:11:40 -0700 Subject: [PATCH 02/12] Add exception for invalid ordinal aliases --- .../sql/rewriter/ordinal/OrdinalRewriterRule.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java index be25280c17..85bd5af691 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java @@ -33,6 +33,7 @@ import com.amazon.opendistroforelasticsearch.sql.rewriter.RewriteRule; import com.amazon.opendistroforelasticsearch.sql.rewriter.RewriteRuleExecutor; import com.amazon.opendistroforelasticsearch.sql.rewriter.alias.TableAliasPrefixRemoveRule; +import com.amazon.opendistroforelasticsearch.sql.rewriter.matchtoterm.VerificationException; import java.sql.SQLFeatureNotSupportedException; import java.util.HashMap; @@ -146,6 +147,11 @@ public boolean visit(MySqlSelectGroupByExpr groupByExpr) { if (expr instanceof SQLIntegerExpr) { Integer ordinalValue = ((SQLIntegerExpr) expr).getNumber().intValue(); SQLExpr newExpr = groupOrdinalToExpr.get(ordinalValue); + if (newExpr == null) { + throw new VerificationException( + "Invalid ordinal [" + ordinalValue + "] specified in [GROUP BY " + ordinalValue +"] " + ); + } groupByExpr.setExpr(newExpr); newExpr.setParent(groupByExpr); } @@ -156,11 +162,16 @@ public boolean visit(MySqlSelectGroupByExpr groupByExpr) { public boolean visit(SQLSelectOrderByItem orderByItem) { SQLExpr expr = orderByItem.getExpr(); Integer ordinalValue; + String exception = "Invalid ordinal [%s] specified in [ORDER BY %s]"; if (expr instanceof SQLIntegerExpr) { ordinalValue = ((SQLIntegerExpr) expr).getNumber().intValue(); SQLExpr newExpr = orderOrdinalToExpr.get(ordinalValue); + if (newExpr == null) { + throw new VerificationException(String.format(exception, ordinalValue, ordinalValue)); + } + orderByItem.setExpr(newExpr); newExpr.setParent(orderByItem); }else if (expr instanceof SQLBinaryOpExpr @@ -171,6 +182,10 @@ public boolean visit(SQLSelectOrderByItem orderByItem) { ordinalValue = integerExpr.getNumber().intValue(); SQLExpr newExpr = orderOrdinalToExpr.get(ordinalValue); + if (newExpr == null) { + throw new VerificationException(String.format(exception, ordinalValue, ordinalValue)); + } + binaryOpExpr.setLeft(newExpr); newExpr.setParent(binaryOpExpr); } From 436fddd4b7fb7c7089166cbc7764ce28d55cfaef Mon Sep 17 00:00:00 2001 From: Hussain Date: Wed, 23 Oct 2019 11:23:44 -0700 Subject: [PATCH 03/12] Add UnquoteIdentifierRule to OrdinalRewriterRule --- .../sql/rewriter/ordinal/OrdinalRewriterRule.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java index 85bd5af691..45c5a1681f 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java @@ -33,6 +33,7 @@ import com.amazon.opendistroforelasticsearch.sql.rewriter.RewriteRule; import com.amazon.opendistroforelasticsearch.sql.rewriter.RewriteRuleExecutor; import com.amazon.opendistroforelasticsearch.sql.rewriter.alias.TableAliasPrefixRemoveRule; +import com.amazon.opendistroforelasticsearch.sql.rewriter.identifier.UnquoteIdentifierRule; import com.amazon.opendistroforelasticsearch.sql.rewriter.matchtoterm.VerificationException; import java.sql.SQLFeatureNotSupportedException; @@ -85,6 +86,7 @@ public boolean match(SQLQueryExpr root) throws SQLFeatureNotSupportedException { // we need to rewrite the generated syntax tree to match the original syntax tree Select clause ruleExecutor = RewriteRuleExecutor.builder() + .withRule(new UnquoteIdentifierRule()) .withRule(new TableAliasPrefixRemoveRule()) .build(); From f9930ce6327d6b808b4b0c86768bfb268f8f79a0 Mon Sep 17 00:00:00 2001 From: Hussain Date: Wed, 23 Oct 2019 12:29:40 -0700 Subject: [PATCH 04/12] Code refactor --- .../rewriter/ordinal/OrdinalRewriterRule.java | 33 +++++++++---------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java index 45c5a1681f..8624749fbd 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java @@ -66,7 +66,7 @@ public boolean match(SQLQueryExpr root) throws SQLFeatureNotSupportedException { MySqlSelectQueryBlock query = (MySqlSelectQueryBlock) sqlSelectQuery; - if (!hasGroupBy(query) && !hasOrderBy(query)) { + if (!hasGroupByWithOrdinals(query) && !hasOrderByWithOrdinals(query)) { return false; } @@ -100,7 +100,7 @@ public void rewrite(SQLQueryExpr root) { changeOrdinalAliasInGroupAndOrderBy(root); } - private boolean hasGroupBy(MySqlSelectQueryBlock query) { + private boolean hasGroupByWithOrdinals(MySqlSelectQueryBlock query) { if (query.getGroupBy() == null) { return false; } else if (query.getGroupBy().getItems().isEmpty()){ @@ -112,7 +112,7 @@ private boolean hasGroupBy(MySqlSelectQueryBlock query) { ); } - private boolean hasOrderBy(MySqlSelectQueryBlock query) { + private boolean hasOrderByWithOrdinals(MySqlSelectQueryBlock query) { if (query.getOrderBy() == null) { return false; } else if (query.getOrderBy().getItems().isEmpty()){ @@ -143,17 +143,16 @@ public boolean visit(SQLSelectItem select) { private void changeOrdinalAliasInGroupAndOrderBy(SQLQueryExpr root) { root.accept(new MySqlASTVisitorAdapter() { + private String groupException = "Invalid ordinal [%s] specified in [GROUP BY %s]"; + private String orderException = "Invalid ordinal [%s] specified in [ORDER BY %s]"; + @Override public boolean visit(MySqlSelectGroupByExpr groupByExpr) { SQLExpr expr = groupByExpr.getExpr(); if (expr instanceof SQLIntegerExpr) { Integer ordinalValue = ((SQLIntegerExpr) expr).getNumber().intValue(); SQLExpr newExpr = groupOrdinalToExpr.get(ordinalValue); - if (newExpr == null) { - throw new VerificationException( - "Invalid ordinal [" + ordinalValue + "] specified in [GROUP BY " + ordinalValue +"] " - ); - } + checkInvalidOrdinal(newExpr, ordinalValue, groupException); groupByExpr.setExpr(newExpr); newExpr.setParent(groupByExpr); } @@ -164,16 +163,11 @@ public boolean visit(MySqlSelectGroupByExpr groupByExpr) { public boolean visit(SQLSelectOrderByItem orderByItem) { SQLExpr expr = orderByItem.getExpr(); Integer ordinalValue; - String exception = "Invalid ordinal [%s] specified in [ORDER BY %s]"; if (expr instanceof SQLIntegerExpr) { ordinalValue = ((SQLIntegerExpr) expr).getNumber().intValue(); SQLExpr newExpr = orderOrdinalToExpr.get(ordinalValue); - - if (newExpr == null) { - throw new VerificationException(String.format(exception, ordinalValue, ordinalValue)); - } - + checkInvalidOrdinal(newExpr, ordinalValue, orderException); orderByItem.setExpr(newExpr); newExpr.setParent(orderByItem); }else if (expr instanceof SQLBinaryOpExpr @@ -184,10 +178,7 @@ public boolean visit(SQLSelectOrderByItem orderByItem) { ordinalValue = integerExpr.getNumber().intValue(); SQLExpr newExpr = orderOrdinalToExpr.get(ordinalValue); - if (newExpr == null) { - throw new VerificationException(String.format(exception, ordinalValue, ordinalValue)); - } - + checkInvalidOrdinal(newExpr, ordinalValue, orderException); binaryOpExpr.setLeft(newExpr); newExpr.setParent(binaryOpExpr); } @@ -210,4 +201,10 @@ private static SQLExpr toSqlExpr(String sql) { } return expr; } + + private void checkInvalidOrdinal(SQLExpr expr, Integer ordinal, String exception){ + if (expr == null) { + throw new VerificationException(String.format(exception, ordinal, ordinal)); + } + } } From befc218a463fc59b7ca1af88640b69fbc560bc85 Mon Sep 17 00:00:00 2001 From: Hussain Date: Wed, 23 Oct 2019 12:40:27 -0700 Subject: [PATCH 05/12] Add unit tests --- .../ordinal/OrdinalRewriterRuleTest.java | 152 ++++++++++++++++++ 1 file changed, 152 insertions(+) create mode 100644 src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java new file mode 100644 index 0000000000..0486d41b95 --- /dev/null +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java @@ -0,0 +1,152 @@ +/* + * 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.rewriter.ordinal; + +import com.alibaba.druid.sql.SQLUtils; +import com.alibaba.druid.sql.ast.expr.SQLQueryExpr; + +import com.amazon.opendistroforelasticsearch.sql.rewriter.ordinal.OrdinalRewriterRule; +import com.amazon.opendistroforelasticsearch.sql.util.SqlParserUtils; +import org.junit.Assert; +import org.junit.Test; + +import java.sql.SQLFeatureNotSupportedException; + +/** + * Test cases for ordinal aliases in GROUP BY and ORDER BY + */ + +public class OrdinalRewriterRuleTest { + + @Test + public void ordinalInGroupByShouldMatch() throws SQLFeatureNotSupportedException { + query("SELECT lastname FROM bank GROUP BY 1").shouldMatchRule(); + } + + @Test + public void ordinalInOrderByShouldMatch() throws SQLFeatureNotSupportedException { + query("SELECT lastname FROM bank ORDER BY 1").shouldMatchRule(); + } + + + @Test + public void ordinalInGroupAndOrderByShouldMatch() throws SQLFeatureNotSupportedException { + query("SELECT lastname, age FROM bank GROUP BY 2, 1 ORDER BY 1").shouldMatchRule(); + } + + @Test + public void noOrdinalInGroupByShouldNotMatch() throws SQLFeatureNotSupportedException { + query("SELECT lastname FROM bank GROUP BY lastname").shouldNotMatchRule(); + } + + @Test + public void noOrdinalInOrderByShouldNotMatch() throws SQLFeatureNotSupportedException { + query("SELECT lastname, age FROM bank ORDER BY age").shouldNotMatchRule(); + } + + @Test + public void noOrdinalInGroupAndOrderByShouldNotMatch() throws SQLFeatureNotSupportedException { + query("SELECT lastname, age FROM bank GROUP BY lastname, age ORDER BY age").shouldNotMatchRule(); + } + + @Test + public void simpleGroupByOrdinal() throws SQLFeatureNotSupportedException { + query("SELECT lastname FROM bank GROUP BY 1" + ).shouldBeAfterRewrite("SELECT lastname FROM bank GROUP BY lastname"); + } + + @Test + public void multipleGroupByOrdinal() throws SQLFeatureNotSupportedException { + query("SELECT lastname, age FROM bank GROUP BY 1, 2 " + ).shouldBeAfterRewrite("SELECT lastname, age FROM bank GROUP BY lastname, age"); + } + + @Test + public void multipleGroupByOrdinalDifferentorder() throws SQLFeatureNotSupportedException { + query("SELECT lastname, age FROM bank GROUP BY 2, 1" + ).shouldBeAfterRewrite("SELECT lastname, age FROM bank GROUP BY age, lastname"); + } + + @Test + public void simpleOrderByOrdinal() throws SQLFeatureNotSupportedException { + query("SELECT lastname FROM bank ORDER BY 1" + ).shouldBeAfterRewrite("SELECT lastname FROM bank ORDER BY lastname"); + } + + @Test + public void multipleOrderByOrdinal() throws SQLFeatureNotSupportedException { + query("SELECT lastname, age FROM bank ORDER BY 1, 2 " + ).shouldBeAfterRewrite("SELECT lastname, age FROM bank ORDER BY lastname, age"); + } + + @Test + public void multipleOrderByOrdinalDifferentorder() throws SQLFeatureNotSupportedException { + query("SELECT lastname, age FROM bank ORDER BY 2, 1" + ).shouldBeAfterRewrite("SELECT lastname, age FROM bank ORDER BY age, lastname"); + } + + + //Tests with table aliases + + + + + //Tests with quoted identifiers + + + + //Tests with invalid ordinals + + + + // Some more Tests + + + private QueryAssertion query(String sql) { + return new QueryAssertion(sql); + } + private static class QueryAssertion { + + private OrdinalRewriterRule rule; + private SQLQueryExpr expr; + + QueryAssertion(String sql) { + this.expr = SqlParserUtils.parse(sql); + this.rule = new OrdinalRewriterRule(sql); + } + + void shouldBeAfterRewrite(String expected) throws SQLFeatureNotSupportedException { + shouldMatchRule(); + rule.rewrite(expr); + Assert.assertEquals( + SQLUtils.toMySqlString(SqlParserUtils.parse(expected)), + SQLUtils.toMySqlString(expr) + ); + } + + void shouldMatchRule() throws SQLFeatureNotSupportedException { + Assert.assertTrue(match()); + } + + void shouldNotMatchRule() throws SQLFeatureNotSupportedException { + Assert.assertFalse(match()); + } + + private boolean match() throws SQLFeatureNotSupportedException { + return rule.match(expr); + } + } +} From 35791e0d609f9342e8d8359d4d95bb76147adf53 Mon Sep 17 00:00:00 2001 From: Hussain Date: Wed, 23 Oct 2019 13:13:51 -0700 Subject: [PATCH 06/12] Exception validation method --- .../ordinal/OrdinalRewriterRuleTest.java | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java index 0486d41b95..a5394a6764 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java @@ -18,13 +18,17 @@ import com.alibaba.druid.sql.SQLUtils; import com.alibaba.druid.sql.ast.expr.SQLQueryExpr; +import com.amazon.opendistroforelasticsearch.sql.rewriter.matchtoterm.VerificationException; import com.amazon.opendistroforelasticsearch.sql.rewriter.ordinal.OrdinalRewriterRule; import com.amazon.opendistroforelasticsearch.sql.util.SqlParserUtils; + import org.junit.Assert; import org.junit.Test; import java.sql.SQLFeatureNotSupportedException; +import static org.hamcrest.Matchers.containsString; + /** * Test cases for ordinal aliases in GROUP BY and ORDER BY */ @@ -97,23 +101,9 @@ public void multipleOrderByOrdinalDifferentorder() throws SQLFeatureNotSupported query("SELECT lastname, age FROM bank ORDER BY 2, 1" ).shouldBeAfterRewrite("SELECT lastname, age FROM bank ORDER BY age, lastname"); } + - - //Tests with table aliases - - - - - //Tests with quoted identifiers - - - - //Tests with invalid ordinals - - - - // Some more Tests - + // TODO: Some more Tests private QueryAssertion query(String sql) { return new QueryAssertion(sql); @@ -145,6 +135,16 @@ void shouldNotMatchRule() throws SQLFeatureNotSupportedException { Assert.assertFalse(match()); } + void shouldThrowException(int ordinal) throws SQLFeatureNotSupportedException { + try { + shouldMatchRule(); + rule.rewrite(expr); + Assert.fail("Expected VerificationException, but none was thrown"); + } catch (VerificationException e) { + Assert.assertThat(e.getMessage(), containsString("Invalid ordinal ["+ ordinal +"] specified in")); + } + } + private boolean match() throws SQLFeatureNotSupportedException { return rule.match(expr); } From 852ac17483dc196b6a523b7a54c281868445480d Mon Sep 17 00:00:00 2001 From: Hussain Date: Wed, 23 Oct 2019 13:27:23 -0700 Subject: [PATCH 07/12] More unit tests --- .../ordinal/OrdinalRewriterRuleTest.java | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java index a5394a6764..3017a39539 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java @@ -76,14 +76,18 @@ public void simpleGroupByOrdinal() throws SQLFeatureNotSupportedException { public void multipleGroupByOrdinal() throws SQLFeatureNotSupportedException { query("SELECT lastname, age FROM bank GROUP BY 1, 2 " ).shouldBeAfterRewrite("SELECT lastname, age FROM bank GROUP BY lastname, age"); - } - @Test - public void multipleGroupByOrdinalDifferentorder() throws SQLFeatureNotSupportedException { query("SELECT lastname, age FROM bank GROUP BY 2, 1" ).shouldBeAfterRewrite("SELECT lastname, age FROM bank GROUP BY age, lastname"); + + query("SELECT lastname, age, firstname FROM bank GROUP BY 2, firstname, 1" + ).shouldBeAfterRewrite("SELECT lastname, age, firstname FROM bank GROUP BY age, firstname, lastname"); + + query("SELECT lastname, age, firstname FROM bank GROUP BY 2, something, 1" + ).shouldBeAfterRewrite("SELECT lastname, age, firstname FROM bank GROUP BY age, something, lastname"); } + @Test public void simpleOrderByOrdinal() throws SQLFeatureNotSupportedException { query("SELECT lastname FROM bank ORDER BY 1" @@ -94,14 +98,17 @@ public void simpleOrderByOrdinal() throws SQLFeatureNotSupportedException { public void multipleOrderByOrdinal() throws SQLFeatureNotSupportedException { query("SELECT lastname, age FROM bank ORDER BY 1, 2 " ).shouldBeAfterRewrite("SELECT lastname, age FROM bank ORDER BY lastname, age"); - } - @Test - public void multipleOrderByOrdinalDifferentorder() throws SQLFeatureNotSupportedException { query("SELECT lastname, age FROM bank ORDER BY 2, 1" ).shouldBeAfterRewrite("SELECT lastname, age FROM bank ORDER BY age, lastname"); + + query("SELECT lastname, age, firstname FROM bank ORDER BY 2, firstname, 1" + ).shouldBeAfterRewrite("SELECT lastname, age, firstname FROM bank ORDER BY age, firstname, lastname"); + + query("SELECT lastname, age, firstname FROM bank ORDER BY 2, department, 1" + ).shouldBeAfterRewrite("SELECT lastname, age, firstname FROM bank ORDER BY age, department, lastname"); } - + // TODO: Some more Tests From 065064f20c6efad91adbe93b980025ae393da23a Mon Sep 17 00:00:00 2001 From: Hussain Date: Wed, 23 Oct 2019 15:36:40 -0700 Subject: [PATCH 08/12] Address some comments --- .../rewriter/ordinal/OrdinalRewriterRule.java | 125 +++++++++--------- 1 file changed, 63 insertions(+), 62 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java index 8624749fbd..fdaa04765f 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java @@ -26,9 +26,7 @@ import com.alibaba.druid.sql.dialect.mysql.ast.expr.MySqlSelectGroupByExpr; import com.alibaba.druid.sql.dialect.mysql.ast.statement.MySqlSelectQueryBlock; import com.alibaba.druid.sql.dialect.mysql.visitor.MySqlASTVisitorAdapter; -import com.alibaba.druid.sql.parser.ParserException; import com.alibaba.druid.sql.parser.SQLExprParser; -import com.alibaba.druid.sql.parser.Token; import com.amazon.opendistroforelasticsearch.sql.parser.ElasticSqlExprParser; import com.amazon.opendistroforelasticsearch.sql.rewriter.RewriteRule; import com.amazon.opendistroforelasticsearch.sql.rewriter.RewriteRuleExecutor; @@ -57,7 +55,7 @@ public OrdinalRewriterRule(String sql) { } @Override - public boolean match(SQLQueryExpr root) throws SQLFeatureNotSupportedException { + public boolean match(SQLQueryExpr root) { SQLSelectQuery sqlSelectQuery = root.getSubQuery().getQuery(); if (!(sqlSelectQuery instanceof MySqlSelectQueryBlock)) { // it could be SQLUnionQuery @@ -70,12 +68,18 @@ public boolean match(SQLQueryExpr root) throws SQLFeatureNotSupportedException { return false; } + return true; + } + + @Override + public void rewrite(SQLQueryExpr root) throws SQLFeatureNotSupportedException { + // we cannot clone SQLSelectItem, so we need similar objects to assign to GroupBy and OrderBy items - sqlExprGroupCopy = generateAST(); - sqlExprOrderCopy = generateAST(); + sqlExprGroupCopy = toSqlExpr(); + sqlExprOrderCopy = toSqlExpr(); - collectOrdinalAliases(sqlExprGroupCopy, groupOrdinalToExpr); - collectOrdinalAliases(sqlExprOrderCopy, orderOrdinalToExpr); + mapOrdinalIndexesToSelectItems(sqlExprGroupCopy, groupOrdinalToExpr); + mapOrdinalIndexesToSelectItems(sqlExprOrderCopy, orderOrdinalToExpr); if (groupOrdinalToExpr.isEmpty()) { // orderOrdinalToExpr and groupOrdinalToExpr should be identical @@ -91,55 +95,11 @@ public boolean match(SQLQueryExpr root) throws SQLFeatureNotSupportedException { .build(); ruleExecutor.executeOn(sqlExprGroupCopy); - ruleExecutor.executeOn(sqlExprGroupCopy); - return true; - } + ruleExecutor.executeOn(sqlExprOrderCopy); - @Override - public void rewrite(SQLQueryExpr root) { changeOrdinalAliasInGroupAndOrderBy(root); } - private boolean hasGroupByWithOrdinals(MySqlSelectQueryBlock query) { - if (query.getGroupBy() == null) { - return false; - } else if (query.getGroupBy().getItems().isEmpty()){ - return false; - } - - return query.getGroupBy().getItems().stream().anyMatch(x -> - x instanceof MySqlSelectGroupByExpr && ((MySqlSelectGroupByExpr) x).getExpr() instanceof SQLIntegerExpr - ); - } - - private boolean hasOrderByWithOrdinals(MySqlSelectQueryBlock query) { - if (query.getOrderBy() == null) { - return false; - } else if (query.getOrderBy().getItems().isEmpty()){ - return false; - } - - return query.getOrderBy().getItems().stream().anyMatch(x -> - x.getExpr() instanceof SQLIntegerExpr - || ( - x.getExpr() instanceof SQLBinaryOpExpr - && ((SQLBinaryOpExpr) x.getExpr()).getLeft() instanceof SQLIntegerExpr - ) - ); - } - - private void collectOrdinalAliases(SQLQueryExpr root, Map map) { - root.accept(new MySqlASTVisitorAdapter() { - private Integer count = 0; - - @Override - public boolean visit(SQLSelectItem select) { - map.put(++count, select.getExpr()); - return false; - } - }); - } - private void changeOrdinalAliasInGroupAndOrderBy(SQLQueryExpr root) { root.accept(new MySqlASTVisitorAdapter() { @@ -170,8 +130,8 @@ public boolean visit(SQLSelectOrderByItem orderByItem) { checkInvalidOrdinal(newExpr, ordinalValue, orderException); orderByItem.setExpr(newExpr); newExpr.setParent(orderByItem); - }else if (expr instanceof SQLBinaryOpExpr - && ((SQLBinaryOpExpr) expr).getLeft() instanceof SQLIntegerExpr) { + } else if (expr instanceof SQLBinaryOpExpr + && ((SQLBinaryOpExpr) expr).getLeft() instanceof SQLIntegerExpr) { // support ORDER BY IS NULL/NOT NULL SQLBinaryOpExpr binaryOpExpr = (SQLBinaryOpExpr) expr; SQLIntegerExpr integerExpr = (SQLIntegerExpr) binaryOpExpr.getLeft(); @@ -188,18 +148,59 @@ public boolean visit(SQLSelectOrderByItem orderByItem) { }); } - private SQLQueryExpr generateAST() { - return (SQLQueryExpr) toSqlExpr(sql); + private void mapOrdinalIndexesToSelectItems(SQLQueryExpr root, Map map) { + root.accept(new MySqlASTVisitorAdapter() { + private Integer count = 0; + + @Override + public boolean visit(SQLSelectItem select) { + map.put(++count, select.getExpr()); + return false; + } + }); } - private static SQLExpr toSqlExpr(String sql) { - SQLExprParser parser = new ElasticSqlExprParser(sql); - SQLExpr expr = parser.expr(); + private boolean hasGroupByWithOrdinals(MySqlSelectQueryBlock query) { + if (query.getGroupBy() == null) { + return false; + } else if (query.getGroupBy().getItems().isEmpty()){ + return false; + } - if (parser.getLexer().token() != Token.EOF) { - throw new ParserException("Illegal SQL expression : " + sql); + return query.getGroupBy().getItems().stream().anyMatch(x -> + x instanceof MySqlSelectGroupByExpr && ((MySqlSelectGroupByExpr) x).getExpr() instanceof SQLIntegerExpr + ); + } + + private boolean hasOrderByWithOrdinals(MySqlSelectQueryBlock query) { + if (query.getOrderBy() == null) { + return false; + } else if (query.getOrderBy().getItems().isEmpty()){ + return false; } - return expr; + + /** + * The second condition checks valid AST that meets ORDER BY IS NULL/NOT NULL condition + * + * SQLSelectOrderByItem + * | + * SQLBinaryOpExpr (Is || IsNot) + * / \ + * SQLIdentifierExpr SQLNullExpr + */ + return query.getOrderBy().getItems().stream().anyMatch(x -> + x.getExpr() instanceof SQLIntegerExpr + || ( + x.getExpr() instanceof SQLBinaryOpExpr + && ((SQLBinaryOpExpr) x.getExpr()).getLeft() instanceof SQLIntegerExpr + ) + ); + } + + private SQLQueryExpr toSqlExpr() { + SQLExprParser parser = new ElasticSqlExprParser(sql); + SQLExpr expr = parser.expr(); + return (SQLQueryExpr) expr; } private void checkInvalidOrdinal(SQLExpr expr, Integer ordinal, String exception){ From d86c0c0bc7b7235302138efe66fa269d03839cd8 Mon Sep 17 00:00:00 2001 From: Hussain Date: Wed, 23 Oct 2019 17:26:12 -0700 Subject: [PATCH 09/12] Address comments --- .../sql/query/ESActionFactory.java | 2 +- .../rewriter/ordinal/OrdinalRewriterRule.java | 80 +++++-------------- .../ordinal/OrdinalRewriterRuleTest.java | 32 ++++---- 3 files changed, 38 insertions(+), 76 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/ESActionFactory.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/ESActionFactory.java index 5b343b6a9f..cf0d1ee9a8 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/ESActionFactory.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/query/ESActionFactory.java @@ -80,10 +80,10 @@ public static QueryAction create(Client client, String sql) throws SqlParseExcep RewriteRuleExecutor ruleExecutor = RewriteRuleExecutor.builder() .withRule(new SQLExprParentSetterRule()) + .withRule(new OrdinalRewriterRule(sql)) .withRule(new UnquoteIdentifierRule()) .withRule(new TableAliasPrefixRemoveRule()) .withRule(new SubQueryRewriteRule()) - .withRule(new OrdinalRewriterRule(sql)) .build(); ruleExecutor.executeOn(sqlExpr); sqlExpr.accept(new NestedFieldRewriter()); diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java index fdaa04765f..dffde40237 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java @@ -13,7 +13,6 @@ * permissions and limitations under the License. */ - package com.amazon.opendistroforelasticsearch.sql.rewriter.ordinal; import com.alibaba.druid.sql.ast.SQLExpr; @@ -29,26 +28,16 @@ import com.alibaba.druid.sql.parser.SQLExprParser; import com.amazon.opendistroforelasticsearch.sql.parser.ElasticSqlExprParser; import com.amazon.opendistroforelasticsearch.sql.rewriter.RewriteRule; -import com.amazon.opendistroforelasticsearch.sql.rewriter.RewriteRuleExecutor; -import com.amazon.opendistroforelasticsearch.sql.rewriter.alias.TableAliasPrefixRemoveRule; -import com.amazon.opendistroforelasticsearch.sql.rewriter.identifier.UnquoteIdentifierRule; import com.amazon.opendistroforelasticsearch.sql.rewriter.matchtoterm.VerificationException; -import java.sql.SQLFeatureNotSupportedException; -import java.util.HashMap; -import java.util.Map; +import java.util.List; /** * Rewrite rule for changing ordinal alias in order by and group by to actual select field. */ public class OrdinalRewriterRule implements RewriteRule { - private SQLQueryExpr sqlExprGroupCopy; - private SQLQueryExpr sqlExprOrderCopy; private final String sql; - private final Map groupOrdinalToExpr = new HashMap<>(); - private final Map orderOrdinalToExpr = new HashMap<>(); - private RewriteRuleExecutor ruleExecutor; public OrdinalRewriterRule(String sql) { this.sql = sql; @@ -67,52 +56,39 @@ public boolean match(SQLQueryExpr root) { if (!hasGroupByWithOrdinals(query) && !hasOrderByWithOrdinals(query)) { return false; } - return true; } @Override - public void rewrite(SQLQueryExpr root) throws SQLFeatureNotSupportedException { + public void rewrite(SQLQueryExpr root) { // we cannot clone SQLSelectItem, so we need similar objects to assign to GroupBy and OrderBy items - sqlExprGroupCopy = toSqlExpr(); - sqlExprOrderCopy = toSqlExpr(); - - mapOrdinalIndexesToSelectItems(sqlExprGroupCopy, groupOrdinalToExpr); - mapOrdinalIndexesToSelectItems(sqlExprOrderCopy, orderOrdinalToExpr); - - if (groupOrdinalToExpr.isEmpty()) { - // orderOrdinalToExpr and groupOrdinalToExpr should be identical - // because they are generated from similar syntax trees - // checking for groupOrdinalToExpr or orderOrdinalToExpr is enough - return false; - } + SQLQueryExpr sqlExprGroupCopy = toSqlExpr(); + SQLQueryExpr sqlExprOrderCopy = toSqlExpr(); - // we need to rewrite the generated syntax tree to match the original syntax tree Select clause - ruleExecutor = RewriteRuleExecutor.builder() - .withRule(new UnquoteIdentifierRule()) - .withRule(new TableAliasPrefixRemoveRule()) - .build(); - - ruleExecutor.executeOn(sqlExprGroupCopy); - ruleExecutor.executeOn(sqlExprOrderCopy); - - changeOrdinalAliasInGroupAndOrderBy(root); + changeOrdinalAliasInGroupAndOrderBy(root, sqlExprGroupCopy, sqlExprOrderCopy); } - private void changeOrdinalAliasInGroupAndOrderBy(SQLQueryExpr root) { + private void changeOrdinalAliasInGroupAndOrderBy(SQLQueryExpr root, + SQLQueryExpr exprGroup, + SQLQueryExpr exprOrder) { root.accept(new MySqlASTVisitorAdapter() { private String groupException = "Invalid ordinal [%s] specified in [GROUP BY %s]"; private String orderException = "Invalid ordinal [%s] specified in [ORDER BY %s]"; + private List groupSelectList = ((MySqlSelectQueryBlock) exprGroup.getSubQuery().getQuery()) + .getSelectList(); + + private List orderSelectList = ((MySqlSelectQueryBlock) exprOrder.getSubQuery().getQuery()) + .getSelectList(); + @Override public boolean visit(MySqlSelectGroupByExpr groupByExpr) { SQLExpr expr = groupByExpr.getExpr(); if (expr instanceof SQLIntegerExpr) { Integer ordinalValue = ((SQLIntegerExpr) expr).getNumber().intValue(); - SQLExpr newExpr = groupOrdinalToExpr.get(ordinalValue); - checkInvalidOrdinal(newExpr, ordinalValue, groupException); + SQLExpr newExpr = checkAndGet(groupSelectList, ordinalValue, groupException); groupByExpr.setExpr(newExpr); newExpr.setParent(groupByExpr); } @@ -126,8 +102,7 @@ public boolean visit(SQLSelectOrderByItem orderByItem) { if (expr instanceof SQLIntegerExpr) { ordinalValue = ((SQLIntegerExpr) expr).getNumber().intValue(); - SQLExpr newExpr = orderOrdinalToExpr.get(ordinalValue); - checkInvalidOrdinal(newExpr, ordinalValue, orderException); + SQLExpr newExpr = checkAndGet(orderSelectList, ordinalValue, orderException); orderByItem.setExpr(newExpr); newExpr.setParent(orderByItem); } else if (expr instanceof SQLBinaryOpExpr @@ -137,8 +112,7 @@ public boolean visit(SQLSelectOrderByItem orderByItem) { SQLIntegerExpr integerExpr = (SQLIntegerExpr) binaryOpExpr.getLeft(); ordinalValue = integerExpr.getNumber().intValue(); - SQLExpr newExpr = orderOrdinalToExpr.get(ordinalValue); - checkInvalidOrdinal(newExpr, ordinalValue, orderException); + SQLExpr newExpr = checkAndGet(orderSelectList, ordinalValue, orderException); binaryOpExpr.setLeft(newExpr); newExpr.setParent(binaryOpExpr); } @@ -148,16 +122,12 @@ public boolean visit(SQLSelectOrderByItem orderByItem) { }); } - private void mapOrdinalIndexesToSelectItems(SQLQueryExpr root, Map map) { - root.accept(new MySqlASTVisitorAdapter() { - private Integer count = 0; + private SQLExpr checkAndGet(List selectList, Integer ordinal, String exception) { + if (ordinal > selectList.size()) { + throw new VerificationException(String.format(exception, ordinal, ordinal)); + } - @Override - public boolean visit(SQLSelectItem select) { - map.put(++count, select.getExpr()); - return false; - } - }); + return selectList.get(ordinal-1).getExpr(); } private boolean hasGroupByWithOrdinals(MySqlSelectQueryBlock query) { @@ -202,10 +172,4 @@ private SQLQueryExpr toSqlExpr() { SQLExpr expr = parser.expr(); return (SQLQueryExpr) expr; } - - private void checkInvalidOrdinal(SQLExpr expr, Integer ordinal, String exception){ - if (expr == null) { - throw new VerificationException(String.format(exception, ordinal, ordinal)); - } - } } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java index 3017a39539..57327d2689 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java @@ -25,8 +25,6 @@ import org.junit.Assert; import org.junit.Test; -import java.sql.SQLFeatureNotSupportedException; - import static org.hamcrest.Matchers.containsString; /** @@ -36,44 +34,44 @@ public class OrdinalRewriterRuleTest { @Test - public void ordinalInGroupByShouldMatch() throws SQLFeatureNotSupportedException { + public void ordinalInGroupByShouldMatch() { query("SELECT lastname FROM bank GROUP BY 1").shouldMatchRule(); } @Test - public void ordinalInOrderByShouldMatch() throws SQLFeatureNotSupportedException { + public void ordinalInOrderByShouldMatch() { query("SELECT lastname FROM bank ORDER BY 1").shouldMatchRule(); } @Test - public void ordinalInGroupAndOrderByShouldMatch() throws SQLFeatureNotSupportedException { + public void ordinalInGroupAndOrderByShouldMatch() { query("SELECT lastname, age FROM bank GROUP BY 2, 1 ORDER BY 1").shouldMatchRule(); } @Test - public void noOrdinalInGroupByShouldNotMatch() throws SQLFeatureNotSupportedException { + public void noOrdinalInGroupByShouldNotMatch() { query("SELECT lastname FROM bank GROUP BY lastname").shouldNotMatchRule(); } @Test - public void noOrdinalInOrderByShouldNotMatch() throws SQLFeatureNotSupportedException { + public void noOrdinalInOrderByShouldNotMatch() { query("SELECT lastname, age FROM bank ORDER BY age").shouldNotMatchRule(); } @Test - public void noOrdinalInGroupAndOrderByShouldNotMatch() throws SQLFeatureNotSupportedException { + public void noOrdinalInGroupAndOrderByShouldNotMatch() { query("SELECT lastname, age FROM bank GROUP BY lastname, age ORDER BY age").shouldNotMatchRule(); } @Test - public void simpleGroupByOrdinal() throws SQLFeatureNotSupportedException { + public void simpleGroupByOrdinal() { query("SELECT lastname FROM bank GROUP BY 1" ).shouldBeAfterRewrite("SELECT lastname FROM bank GROUP BY lastname"); } @Test - public void multipleGroupByOrdinal() throws SQLFeatureNotSupportedException { + public void multipleGroupByOrdinal() { query("SELECT lastname, age FROM bank GROUP BY 1, 2 " ).shouldBeAfterRewrite("SELECT lastname, age FROM bank GROUP BY lastname, age"); @@ -89,13 +87,13 @@ public void multipleGroupByOrdinal() throws SQLFeatureNotSupportedException { @Test - public void simpleOrderByOrdinal() throws SQLFeatureNotSupportedException { + public void simpleOrderByOrdinal() { query("SELECT lastname FROM bank ORDER BY 1" ).shouldBeAfterRewrite("SELECT lastname FROM bank ORDER BY lastname"); } @Test - public void multipleOrderByOrdinal() throws SQLFeatureNotSupportedException { + public void multipleOrderByOrdinal() { query("SELECT lastname, age FROM bank ORDER BY 1, 2 " ).shouldBeAfterRewrite("SELECT lastname, age FROM bank ORDER BY lastname, age"); @@ -125,7 +123,7 @@ private static class QueryAssertion { this.rule = new OrdinalRewriterRule(sql); } - void shouldBeAfterRewrite(String expected) throws SQLFeatureNotSupportedException { + void shouldBeAfterRewrite(String expected) { shouldMatchRule(); rule.rewrite(expr); Assert.assertEquals( @@ -134,15 +132,15 @@ void shouldBeAfterRewrite(String expected) throws SQLFeatureNotSupportedExceptio ); } - void shouldMatchRule() throws SQLFeatureNotSupportedException { + void shouldMatchRule() { Assert.assertTrue(match()); } - void shouldNotMatchRule() throws SQLFeatureNotSupportedException { + void shouldNotMatchRule() { Assert.assertFalse(match()); } - void shouldThrowException(int ordinal) throws SQLFeatureNotSupportedException { + void shouldThrowException(int ordinal) { try { shouldMatchRule(); rule.rewrite(expr); @@ -152,7 +150,7 @@ void shouldThrowException(int ordinal) throws SQLFeatureNotSupportedException { } } - private boolean match() throws SQLFeatureNotSupportedException { + private boolean match() { return rule.match(expr); } } From 9d5375838b541760c4ad3e1690267168966cb77b Mon Sep 17 00:00:00 2001 From: Hussain Date: Thu, 24 Oct 2019 11:24:39 -0700 Subject: [PATCH 10/12] Address comments: refactor code, remove map, state variables --- .../rewriter/ordinal/OrdinalRewriterRule.java | 9 +++-- .../ordinal/OrdinalRewriterRuleTest.java | 36 ++++++++++++------- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java index dffde40237..6c92984146 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java @@ -34,7 +34,14 @@ /** * Rewrite rule for changing ordinal alias in order by and group by to actual select field. + * Since we cannot clone or deepcopy the Druid SQL objects, we need to generate the + * two syntax tree from the original query to map Group By and Order By fields with ordinal alias + * to Select fields in newly generated syntax tree. + * + * This rewriter assumes that all the backticks have been removed from identifiers. + * It also assumes that table alias have been removed from SELECT, WHERE, GROUP BY, ORDER BY fields. */ + public class OrdinalRewriterRule implements RewriteRule { private final String sql; @@ -52,7 +59,6 @@ public boolean match(SQLQueryExpr root) { } MySqlSelectQueryBlock query = (MySqlSelectQueryBlock) sqlSelectQuery; - if (!hasGroupByWithOrdinals(query) && !hasOrderByWithOrdinals(query)) { return false; } @@ -61,7 +67,6 @@ public boolean match(SQLQueryExpr root) { @Override public void rewrite(SQLQueryExpr root) { - // we cannot clone SQLSelectItem, so we need similar objects to assign to GroupBy and OrderBy items SQLQueryExpr sqlExprGroupCopy = toSqlExpr(); SQLQueryExpr sqlExprOrderCopy = toSqlExpr(); diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java index 57327d2689..3656b976a8 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java @@ -23,9 +23,9 @@ import com.amazon.opendistroforelasticsearch.sql.util.SqlParserUtils; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; - -import static org.hamcrest.Matchers.containsString; +import org.junit.rules.ExpectedException; /** * Test cases for ordinal aliases in GROUP BY and ORDER BY @@ -33,6 +33,9 @@ public class OrdinalRewriterRuleTest { + @Rule + public ExpectedException exception = ExpectedException.none(); + @Test public void ordinalInGroupByShouldMatch() { query("SELECT lastname FROM bank GROUP BY 1").shouldMatchRule(); @@ -43,7 +46,6 @@ public void ordinalInOrderByShouldMatch() { query("SELECT lastname FROM bank ORDER BY 1").shouldMatchRule(); } - @Test public void ordinalInGroupAndOrderByShouldMatch() { query("SELECT lastname, age FROM bank GROUP BY 2, 1 ORDER BY 1").shouldMatchRule(); @@ -83,8 +85,8 @@ public void multipleGroupByOrdinal() { query("SELECT lastname, age, firstname FROM bank GROUP BY 2, something, 1" ).shouldBeAfterRewrite("SELECT lastname, age, firstname FROM bank GROUP BY age, something, lastname"); - } + } @Test public void simpleOrderByOrdinal() { @@ -107,8 +109,21 @@ public void multipleOrderByOrdinal() { ).shouldBeAfterRewrite("SELECT lastname, age, firstname FROM bank ORDER BY age, department, lastname"); } + // Tests invalid Ordinals, non-positive ordinal values are already validated by semantic analyzer + @Test + public void invalidGroupByOrdinalShouldThrowException() { + exception.expect(VerificationException.class); + exception.expectMessage("Invalid ordinal [3] specified in [GROUP BY 3]"); + query("SELECT lastname, MAX(lastname) FROM bank GROUP BY 3 ").rewrite(); + } + + @Test + public void invalidOrderByOrdinalShouldThrowException() { + exception.expect(VerificationException.class); + exception.expectMessage("Invalid ordinal [4] specified in [ORDER BY 4]"); + query("SELECT `lastname`, `age`, `firstname` FROM bank ORDER BY 4 IS NOT NULL").rewrite(); + } - // TODO: Some more Tests private QueryAssertion query(String sql) { return new QueryAssertion(sql); @@ -140,14 +155,9 @@ void shouldNotMatchRule() { Assert.assertFalse(match()); } - void shouldThrowException(int ordinal) { - try { - shouldMatchRule(); - rule.rewrite(expr); - Assert.fail("Expected VerificationException, but none was thrown"); - } catch (VerificationException e) { - Assert.assertThat(e.getMessage(), containsString("Invalid ordinal ["+ ordinal +"] specified in")); - } + void rewrite() { + shouldMatchRule(); + rule.rewrite(expr); } private boolean match() { From 762ff1058a1791af03609c0742e3bbc440022a98 Mon Sep 17 00:00:00 2001 From: Hussain Date: Thu, 24 Oct 2019 11:34:04 -0700 Subject: [PATCH 11/12] Added integration tests --- .../esintgtest/OrdinalAliasRewriterIT.java | 167 ++++++++++++++++++ 1 file changed, 167 insertions(+) create mode 100644 src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/OrdinalAliasRewriterIT.java diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/OrdinalAliasRewriterIT.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/OrdinalAliasRewriterIT.java new file mode 100644 index 0000000000..2f5c531954 --- /dev/null +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/OrdinalAliasRewriterIT.java @@ -0,0 +1,167 @@ +/* + * 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.esintgtest; + +import com.amazon.opendistroforelasticsearch.sql.utils.StringUtils; +import org.junit.Test; + +import java.io.IOException; + +import static org.hamcrest.Matchers.equalTo; + +public class OrdinalAliasRewriterIT extends SQLIntegTestCase { + + @Override + protected void init() throws Exception { + loadIndex(Index.ACCOUNT); + } + + // tests query results with jdbc output + @Test + public void simpleGroupByOrdinal() { + String expected = executeQuery(StringUtils.format( + "SELECT lastname FROM %s AS b GROUP BY lastname LIMIT 3", TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + String actual = executeQuery(StringUtils.format( + "SELECT lastname FROM %s AS b GROUP BY 1 LIMIT 3", TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + assertThat(actual, equalTo(expected)); + } + + @Test + public void multipleGroupByOrdinal() { + String expected = executeQuery(StringUtils.format( + "SELECT lastname, firstname, age FROM %s AS b GROUP BY firstname, age, lastname LIMIT 3", + TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + String actual = executeQuery(StringUtils.format( + "SELECT lastname, firstname, age FROM %s AS b GROUP BY 2, 3, 1 LIMIT 3", + TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + assertThat(actual, equalTo(expected)); + } + + + @Test + public void sometest() { + String expected = executeQuery(StringUtils.format( + "SELECT `lastname` FROM %s AS b GROUP BY `lastname` LIMIT 3", TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + } + + @Test + public void selectFieldiWithBacticksGroupByOrdinal() { + String expected = executeQuery(StringUtils.format( + "SELECT `lastname` FROM %s AS b GROUP BY `lastname` LIMIT 3", TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + String actual = executeQuery(StringUtils.format( + "SELECT `lastname` FROM %s AS b GROUP BY 1 LIMIT 3", TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + assertThat(actual, equalTo(expected)); + } + + @Test + public void selectFieldiWithBacticksAndTableAliasGroupByOrdinal() { + String expected = executeQuery(StringUtils.format( + "SELECT `b`.`lastname`, `age`, firstname FROM %s AS b GROUP BY `age`, `b`.`lastname` , firstname LIMIT 10", + TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + String actual = executeQuery(StringUtils.format( + "SELECT `b`.`lastname`, `age`, firstname FROM %s AS b GROUP BY 2, 1, 3 LIMIT 10", + TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + assertThat(actual, equalTo(expected)); + } + + @Test + public void simpleOrderByOrdinal() { + String expected = executeQuery(StringUtils.format( + "SELECT lastname FROM %s AS b ORDER BY lastname LIMIT 3", TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + String actual = executeQuery(StringUtils.format( + "SELECT lastname FROM %s AS b ORDER BY 1 LIMIT 3", TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + assertThat(actual, equalTo(expected)); + } + + @Test + public void multipleOrderByOrdinal() { + String expected = executeQuery(StringUtils.format( + "SELECT lastname, firstname, age FROM %s AS b ORDER BY firstname, age, lastname LIMIT 3", + TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + String actual = executeQuery(StringUtils.format( + "SELECT lastname, firstname, age FROM %s AS b ORDER BY 2, 3, 1 LIMIT 3", + TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + assertThat(actual, equalTo(expected)); + } + + @Test + public void selectFieldiWithBacticksOrderByOrdinal() { + String expected = executeQuery(StringUtils.format( + "SELECT `lastname` FROM %s AS b ORDER BY `lastname` LIMIT 3", TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + String actual = executeQuery(StringUtils.format( + "SELECT `lastname` FROM %s AS b ORDER BY 1 LIMIT 3", TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + assertThat(actual, equalTo(expected)); + } + + @Test + public void selectFieldiWithBacticksAndTableAliasOrderByOrdinal() { + String expected = executeQuery(StringUtils.format( + "SELECT `b`.`lastname` FROM %s AS b ORDER BY `b`.`lastname` LIMIT 3", + TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + String actual = executeQuery(StringUtils.format( + "SELECT `b`.`lastname` FROM %s AS b ORDER BY 1 LIMIT 3", + TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + assertThat(actual, equalTo(expected)); + } + + // ORDER BY IS NULL/NOT NULL + @Test + public void selectFieldiWithBacticksAndTableAliasOrderByOrdinalAndNull() { + String expected = executeQuery(StringUtils.format( + "SELECT `b`.`lastname`, age FROM %s AS b ORDER BY `b`.`lastname` IS NOT NULL DESC, age is NULL LIMIT 3", + TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + String actual = executeQuery(StringUtils.format( + "SELECT `b`.`lastname`, age FROM %s AS b ORDER BY 1 IS NOT NULL DESC, 2 IS NULL LIMIT 3", + TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); + assertThat(actual, equalTo(expected)); + } + + + // explain + @Test + public void explainSelectFieldiWithBacticksAndTableAliasGroupByOrdinal() throws IOException { + String expected = explainQuery(StringUtils.format( + "SELECT `b`.`lastname` FROM %s AS b GROUP BY `b`.`lastname` LIMIT 3", + TestsConstants.TEST_INDEX_ACCOUNT)); + String actual = explainQuery(StringUtils.format( + "SELECT `b`.`lastname` FROM %s AS b GROUP BY 1 LIMIT 3", + TestsConstants.TEST_INDEX_ACCOUNT)); + assertThat(actual, equalTo(expected)); + } + + @Test + public void explainSelectFieldiWithBacticksAndTableAliasOrderByOrdinal() throws IOException { + String expected = explainQuery(StringUtils.format( + "SELECT `b`.`lastname` FROM %s AS b ORDER BY `b`.`lastname` LIMIT 3", + TestsConstants.TEST_INDEX_ACCOUNT)); + String actual = explainQuery(StringUtils.format( + "SELECT `b`.`lastname` FROM %s AS b ORDER BY 1 LIMIT 3", + TestsConstants.TEST_INDEX_ACCOUNT)); + assertThat(actual, equalTo(expected)); + } + + // explain ORDER BY IS NULL/NOT NULL + @Test + public void explainSelectFieldiWithBacticksAndTableAliasOrderByOrdinalAndNull() throws IOException { + String expected = explainQuery(StringUtils.format( + "SELECT `b`.`lastname`, age FROM %s AS b ORDER BY `b`.`lastname` IS NOT NULL DESC, age is NULL LIMIT 3", + TestsConstants.TEST_INDEX_ACCOUNT)); + String actual = explainQuery(StringUtils.format( + "SELECT `b`.`lastname`, age FROM %s AS b ORDER BY 1 IS NOT NULL DESC, 2 IS NULL LIMIT 3", + TestsConstants.TEST_INDEX_ACCOUNT)); + assertThat(actual, equalTo(expected)); + } +} From 91191dcbedaa34f0b9efc63aaabf232c905ca45c Mon Sep 17 00:00:00 2001 From: Hussain Date: Thu, 24 Oct 2019 11:50:14 -0700 Subject: [PATCH 12/12] Remove test case used for debugging --- .../sql/esintgtest/OrdinalAliasRewriterIT.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/OrdinalAliasRewriterIT.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/OrdinalAliasRewriterIT.java index 2f5c531954..5b7d729304 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/OrdinalAliasRewriterIT.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/OrdinalAliasRewriterIT.java @@ -50,13 +50,6 @@ public void multipleGroupByOrdinal() { assertThat(actual, equalTo(expected)); } - - @Test - public void sometest() { - String expected = executeQuery(StringUtils.format( - "SELECT `lastname` FROM %s AS b GROUP BY `lastname` LIMIT 3", TestsConstants.TEST_INDEX_ACCOUNT), "jdbc"); - } - @Test public void selectFieldiWithBacticksGroupByOrdinal() { String expected = executeQuery(StringUtils.format(