Skip to content

Commit

Permalink
SQL: Return error with ORDER BY on non-grouped.
Browse files Browse the repository at this point in the history 
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: elastic#34590
  • Loading branch information
@matriv
matriv committed Oct 25, 2018
1 parent a69c540 commit 48720bb
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 38 deletions.
79 changes: 41 additions & 38 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
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;
Expand Down Expand Up @@ -238,52 +239,54 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set<Failure> localFailur
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();

Map<Expression, Node<?>> missing = new LinkedHashMap<>();
o.order().forEach(oe -> {
Expression e = oe.child();
// cannot order by aggregates (not supported by composite)
if (Functions.isAggregate(e)) {
missing.put(e, oe);
return;
}
o.forEachDown(child -> {
if (child instanceof Aggregate) {
Aggregate a = (Aggregate) child;

// 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;
if (Expressions.anyMatch(a.groupings(), g -> Expressions.equalsAsAttribute(al.child(), g))) {
groupingAndMatchingAggregatesAliases.add(al);
}
Map<Expression, Node<?>> missing = new LinkedHashMap<>();
o.order().forEach(oe -> {
Expression e = oe.child();
// cannot order by aggregates (not supported by composite)
if (Functions.isAggregate(e)) {
missing.put(e, oe);
return;
}
});

// make sure to compare attributes directly
if (Expressions.anyMatch(groupingAndMatchingAggregatesAliases,

// 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;
if (Expressions.anyMatch(a.groupings(), g -> Expressions.equalsAsAttribute(al.child(), g))) {
groupingAndMatchingAggregatesAliases.add(al);
}
}
});

// make sure to compare attributes directly
if (Expressions.anyMatch(groupingAndMatchingAggregatesAliases,
g -> e.semanticEquals(e instanceof Attribute ? Expressions.attribute(g) : g))) {
return;
}
return;
}

// nothing matched, cannot group by it
missing.put(e, oe);
});
// nothing matched, cannot group by it
missing.put(e, oe);
});

if (!missing.isEmpty()) {
String plural = missing.size() > 1 ? "s" : StringUtils.EMPTY;
// get the location of the first missing expression as the order by might be on a different line
localFailures.add(
if (!missing.isEmpty()) {
String plural = missing.size() > 1 ? "s" : StringUtils.EMPTY;
// get the location of the first missing expression as the order by might be on a different line
localFailures.add(
fail(missing.values().iterator().next(), "Cannot order by non-grouped column" + plural + " %s, expected %s",
Expressions.names(missing.keySet()),
Expressions.names(a.groupings())));
groupingFailures.add(a);
return false;
Expressions.names(missing.keySet()),
Expressions.names(a.groupings())));
groupingFailures.add(a);
}
}
}
});
}
return true;
}
Expand Down
10 changes: 10 additions & 0 deletions ...c/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ public void testGroupByOrderByNonGrouped() {
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);
Expand All @@ -128,6 +133,11 @@ public void testGroupByOrderByScalarOverNonGrouped() {
verify("SELECT MAX(int) FROM test GROUP BY text ORDER BY YEAR(date)"));
}

public void testGroupByOrderByScalarÎOverNonGrouped_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"));
Expand Down

0 comments on commit 48720bb

Please sign in to comment.