Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL: Verifier allows aliases aggregates for sorting #34773

Merged
merged 2 commits into from
Oct 24, 2018

Conversation

costin
Copy link
Member

@costin costin commented Oct 23, 2018

Improve Verifier rule that prevented grouping, aliases inside aggregates
to not be accepted for ordering.

Close #34607

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@costin costin requested review from astefan and matriv October 24, 2018 10:48
Improve Verifier rule that prevented grouping, aliases inside aggregates
to not be accepted for ordering.

Close elastic#34607
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

LGTM. Left a comment.

@@ -110,6 +118,11 @@ public void testGroupByOrderByNonGrouped() {
verify("SELECT MAX(int) FROM test GROUP BY text ORDER BY bool"));
}

public void testGroupByOrderByAliasedInSelectAllowed() {
LogicalPlan lp = accepted("SELECT text t FROM test GROUP BY text ORDER BY t");
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't group by the alias t but you use text, was that the intention? Because here you use the alias: https://github.com/elastic/elasticsearch/pull/34773/files#diff-3d7c2dc164e7f948316c325f88c12a3fR39

Copy link
Member Author

Choose a reason for hiding this comment

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

The verifier query is correct, the sql isn't (it's a typo). Fixing this in a follow-up commit.

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM

@costin costin merged commit f19565c into elastic:master Oct 24, 2018
@costin costin deleted the fix-34607 branch October 24, 2018 16:31
costin added a commit that referenced this pull request Oct 24, 2018
Improve Verifier rule that prevented grouping with aliases inside aggregates
to not be accepted for ordering.

Close #34607

(cherry picked from commit f19565c)
kcm pushed a commit that referenced this pull request Oct 30, 2018
Improve Verifier rule that prevented grouping with aliases inside aggregates
to not be accepted for ordering.

Close #34607
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants