honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

SQL: Return error with ORDER BY on non-grouped. (#34855) 

Previously, for some queries the validation for ORDER BY
fields didn't kick in since a HAVING close or an ORDER BY
with scalar function would add `Filter` and `Project` plans
between the `OrderBy` and the `Aggregate`.

Now the LogicalPlan tree beneath `OrderBy` is traversed and
the ORDER BY fields are properly verified.

Fixes: #34590
This commit is contained in:
Marios Trivyzas 2018-10-25 22:15:28 +02:00 committed by GitHub
parent cf9aff954e
commit 06d7431950
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 30 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java
View File

@ -25,6 +25,7 @@ import org.elasticsearch.xpack.sql.plan.logical.Distinct;
import org.elasticsearch.xpack.sql.plan.logical.Filter;
import org.elasticsearch.xpack.sql.plan.logical.LogicalPlan;
import org.elasticsearch.xpack.sql.plan.logical.OrderBy;
import org.elasticsearch.xpack.sql.plan.logical.Project;
import org.elasticsearch.xpack.sql.tree.Node;
import org.elasticsearch.xpack.sql.type.DataType;
import org.elasticsearch.xpack.sql.util.StringUtils;
@ -238,8 +239,17 @@ final class Verifier {
Set<LogicalPlan> groupingFailures, Map<String, Function> functions) {
if (p instanceof OrderBy) {
OrderBy o = (OrderBy) p;
if (o.child() instanceof Aggregate) {
Aggregate a = (Aggregate) o.child();
LogicalPlan child = o.child();
if (child instanceof Project) {
child = ((Project) child).child();
}
if (child instanceof Filter) {
child = ((Filter) child).child();
}
if (child instanceof Aggregate) {
Aggregate a = (Aggregate) child;
Map<Expression, Node<?>> missing = new LinkedHashMap<>();
o.order().forEach(oe -> {
@ -253,7 +263,7 @@ final class Verifier {
// take aliases declared inside the aggregates which point to the grouping (but are not included in there)
// to correlate them to the order
List<Expression> groupingAndMatchingAggregatesAliases = new ArrayList<>(a.groupings());
a.aggregates().forEach(as -> {
if (as instanceof Alias) {
Alias al = (Alias) as;
@ -262,10 +272,13 @@ final class Verifier {
}
}
});
// make sure to compare attributes directly
if (Expressions.anyMatch(groupingAndMatchingAggregatesAliases,
g -> e.semanticEquals(e instanceof Attribute ? Expressions.attribute(g) : g))) {
// Make sure you can apply functions on top of the grouped by expressions in the ORDER BY:
// e.g.: if "GROUP BY f2(f1(field))" you can "ORDER BY f4(f3(f2(f1(field))))"
//
// Also, make sure to compare attributes directly
if (e.anyMatch(expression -> Expressions.anyMatch(groupingAndMatchingAggregatesAliases,
g -> expression.semanticEquals(expression instanceof Attribute ? Expressions.attribute(g) : g)))) {
return;
}
@ -288,7 +301,6 @@ final class Verifier {
return true;
}
private static boolean checkGroupByHaving(LogicalPlan p, Set<Failure> localFailures,
Set<LogicalPlan> groupingFailures, Map<String, Function> functions) {
if (p instanceof Filter) {

10
x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java
View File

@ -118,6 +118,11 @@ public class VerifierErrorMessagesTests extends ESTestCase {
verify("SELECT MAX(int) FROM test GROUP BY text ORDER BY bool"));
}
public void testGroupByOrderByNonGrouped_WithHaving() {
assertEquals("1:71: Cannot order by non-grouped column [bool], expected [text]",
verify("SELECT MAX(int) FROM test GROUP BY text HAVING MAX(int) > 10 ORDER BY bool"));
}
public void testGroupByOrderByAliasedInSelectAllowed() {
LogicalPlan lp = accepted("SELECT text t FROM test GROUP BY text ORDER BY t");
assertNotNull(lp);
@ -128,6 +133,11 @@ public class VerifierErrorMessagesTests extends ESTestCase {
verify("SELECT MAX(int) FROM test GROUP BY text ORDER BY YEAR(date)"));
}
public void testGroupByOrderByScalarOverNonGrouped_WithHaving() {
assertEquals("1:71: Cannot order by non-grouped column [YEAR(date [UTC])], expected [text]",
verify("SELECT MAX(int) FROM test GROUP BY text HAVING MAX(int) > 10 ORDER BY YEAR(date)"));
}
public void testGroupByHavingNonGrouped() {
assertEquals("1:48: Cannot filter by non-grouped column [int], expected [text]",
verify("SELECT AVG(int) FROM test GROUP BY text HAVING int > 10"));