Support ordinal aliases in GROUP and ORDER BY clauses #248
Support ordinal aliases in GROUP and ORDER BY clauses #248
Conversation
...ain/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java
Outdated
Show resolved
Hide resolved
I like this approach. In theory, it could be used to inline/fix aliases as well, in order by/group by expressions, instead of hackier approach that we have now (something that I've tried to do in https://github.com/opendistro-for-elasticsearch/sql/pull/168/files, that I needed to revert later) |
...ain/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java
Outdated
Show resolved
Hide resolved
...ain/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java
Outdated
Show resolved
Hide resolved
|
||
MySqlSelectQueryBlock query = (MySqlSelectQueryBlock) sqlSelectQuery; | ||
|
||
if (!hasGroupByWithOrdinals(query) && !hasOrderByWithOrdinals(query)) { |
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.
...ain/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java
Outdated
Show resolved
Hide resolved
...ain/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java
Outdated
Show resolved
Hide resolved
...ain/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java
Outdated
Show resolved
Hide resolved
SQLExpr newExpr = orderOrdinalToExpr.get(ordinalValue); | ||
checkInvalidOrdinal(newExpr, ordinalValue, orderException); |
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.
this is a bit repetitive, you can extract this to method like
getExpressionByOrdinal
and throw an exception inside (will simplify code a bt). Same for visiting group by expressions
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.
Refactored it into checkAndGet
method.
...ain/java/com/amazon/opendistroforelasticsearch/sql/rewriter/ordinal/OrdinalRewriterRule.java
Outdated
Show resolved
Hide resolved
...amazon/opendistroforelasticsearch/sql/unittest/rewriter/ordinal/OrdinalRewriterRuleTest.java
Show resolved
Hide resolved
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.
LGTM
Issue #215
Description of changes:
Testing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.