This repository has been archived by the owner on Aug 2, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 186
Support ordinal aliases in GROUP and ORDER BY clauses #248
Merged
abbashus
merged 15 commits into
opendistro-for-elasticsearch:master
from
abbashus:ordinal-alias
Oct 24, 2019
Merged
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
b5922e5
Support for ordinal alias in GROUP BY and ORDER BY clause
abbashus 69edbf4
Add exception for invalid ordinal aliases
abbashus 543cd0b
Merge branch 'master' into ordinal-alias
abbashus 436fddd
Add UnquoteIdentifierRule to OrdinalRewriterRule
abbashus f9930ce
Code refactor
abbashus befc218
Add unit tests
abbashus 35791e0
Exception validation method
abbashus 852ac17
More unit tests
abbashus 065064f
Address some comments
abbashus b35e7b5
Merge branch 'master' into ordinal-alias
abbashus d86c0c0
Address comments
abbashus 9d53758
Address comments: refactor code, remove map, state variables
abbashus 762ff10
Added integration tests
abbashus d9d6fc5
Merge branch 'master' into ordinal-alias
abbashus 91191dc
Remove test case used for debugging
abbashus File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
175 changes: 175 additions & 0 deletions
175
.../java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,175 @@ | ||
/* | ||
* 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.SQLExprParser; | ||
import com.amazon.opendistroforelasticsearch.sql.parser.ElasticSqlExprParser; | ||
import com.amazon.opendistroforelasticsearch.sql.rewriter.RewriteRule; | ||
import com.amazon.opendistroforelasticsearch.sql.rewriter.matchtoterm.VerificationException; | ||
|
||
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<SQLQueryExpr> { | ||
|
||
private final String sql; | ||
|
||
public OrdinalRewriterRule(String sql) { | ||
this.sql = sql; | ||
} | ||
|
||
@Override | ||
public boolean match(SQLQueryExpr root) { | ||
SQLSelectQuery sqlSelectQuery = root.getSubQuery().getQuery(); | ||
if (!(sqlSelectQuery instanceof MySqlSelectQueryBlock)) { | ||
// it could be SQLUnionQuery | ||
return false; | ||
} | ||
|
||
MySqlSelectQueryBlock query = (MySqlSelectQueryBlock) sqlSelectQuery; | ||
|
||
if (!hasGroupByWithOrdinals(query) && !hasOrderByWithOrdinals(query)) { | ||
return false; | ||
} | ||
return true; | ||
} | ||
|
||
@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(); | ||
|
||
changeOrdinalAliasInGroupAndOrderBy(root, sqlExprGroupCopy, sqlExprOrderCopy); | ||
} | ||
|
||
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<SQLSelectItem> groupSelectList = ((MySqlSelectQueryBlock) exprGroup.getSubQuery().getQuery()) | ||
.getSelectList(); | ||
|
||
private List<SQLSelectItem> 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 = checkAndGet(groupSelectList, ordinalValue, groupException); | ||
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 = checkAndGet(orderSelectList, ordinalValue, orderException); | ||
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 = checkAndGet(orderSelectList, ordinalValue, orderException); | ||
binaryOpExpr.setLeft(newExpr); | ||
newExpr.setParent(binaryOpExpr); | ||
} | ||
|
||
return false; | ||
} | ||
}); | ||
} | ||
|
||
private SQLExpr checkAndGet(List<SQLSelectItem> selectList, Integer ordinal, String exception) { | ||
if (ordinal > selectList.size()) { | ||
throw new VerificationException(String.format(exception, ordinal, ordinal)); | ||
} | ||
|
||
return selectList.get(ordinal-1).getExpr(); | ||
} | ||
|
||
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; | ||
} | ||
|
||
/** | ||
* 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; | ||
} | ||
} |
157 changes: 157 additions & 0 deletions
157
...zon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,157 @@ | ||
/* | ||
* 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.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 static org.hamcrest.Matchers.containsString; | ||
|
||
/** | ||
* Test cases for ordinal aliases in GROUP BY and ORDER BY | ||
*/ | ||
|
||
public class OrdinalRewriterRuleTest { | ||
|
||
abbashus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
@Test | ||
public void ordinalInGroupByShouldMatch() { | ||
query("SELECT lastname FROM bank GROUP BY 1").shouldMatchRule(); | ||
} | ||
|
||
@Test | ||
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(); | ||
} | ||
|
||
@Test | ||
public void noOrdinalInGroupByShouldNotMatch() { | ||
query("SELECT lastname FROM bank GROUP BY lastname").shouldNotMatchRule(); | ||
} | ||
|
||
@Test | ||
public void noOrdinalInOrderByShouldNotMatch() { | ||
query("SELECT lastname, age FROM bank ORDER BY age").shouldNotMatchRule(); | ||
} | ||
|
||
@Test | ||
public void noOrdinalInGroupAndOrderByShouldNotMatch() { | ||
query("SELECT lastname, age FROM bank GROUP BY lastname, age ORDER BY age").shouldNotMatchRule(); | ||
} | ||
|
||
@Test | ||
public void simpleGroupByOrdinal() { | ||
query("SELECT lastname FROM bank GROUP BY 1" | ||
).shouldBeAfterRewrite("SELECT lastname FROM bank GROUP BY lastname"); | ||
} | ||
|
||
@Test | ||
public void multipleGroupByOrdinal() { | ||
query("SELECT lastname, age FROM bank GROUP BY 1, 2 " | ||
).shouldBeAfterRewrite("SELECT lastname, age FROM bank GROUP BY lastname, age"); | ||
|
||
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() { | ||
query("SELECT lastname FROM bank ORDER BY 1" | ||
).shouldBeAfterRewrite("SELECT lastname FROM bank ORDER BY lastname"); | ||
} | ||
|
||
@Test | ||
public void multipleOrderByOrdinal() { | ||
query("SELECT lastname, age FROM bank ORDER BY 1, 2 " | ||
).shouldBeAfterRewrite("SELECT lastname, age FROM bank ORDER BY lastname, age"); | ||
|
||
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 | ||
|
||
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) { | ||
shouldMatchRule(); | ||
rule.rewrite(expr); | ||
Assert.assertEquals( | ||
SQLUtils.toMySqlString(SqlParserUtils.parse(expected)), | ||
SQLUtils.toMySqlString(expr) | ||
); | ||
} | ||
|
||
void shouldMatchRule() { | ||
Assert.assertTrue(match()); | ||
} | ||
|
||
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")); | ||
} | ||
} | ||
|
||
private boolean match() { | ||
return rule.match(expr); | ||
} | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we've supposedly have the same check twice, could you add more comments explaining what's going on? From the code it looks like this checks if we have ordinals, and the code below does the same, but in different way, on parsed raw sql query. Could only one check work? Why we need to parse sql again?
In general, could you add better description of algorithm into the PR or into javadoc, with examples. I find the intention of the code a bit hard to follow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments to help understand the code better. Removed the redundant check.