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 all 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
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
180 changes: 180 additions & 0 deletions
180
.../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,180 @@ | ||
/* | ||
* 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. | ||
* 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<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; | ||
} | ||
} |
160 changes: 160 additions & 0 deletions
160
...est/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/OrdinalAliasRewriterIT.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,160 @@ | ||
/* | ||
* 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 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)); | ||
} | ||
} |
Oops, something went wrong.
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.