Skip to content

Commit

Permalink
SQL: Verifier allows aliases aggregates for sorting (#34773)
Browse files Browse the repository at this point in the history
Improve Verifier rule that prevented grouping with aliases inside aggregates
to not be accepted for ordering.

Close #34607

(cherry picked from commit f19565c)
  • Loading branch information
costin committed Oct 24, 2018
1 parent d6cf565 commit f1d63a7
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package org.elasticsearch.xpack.sql.analysis.analyzer;

import org.elasticsearch.xpack.sql.capabilities.Unresolvable;
import org.elasticsearch.xpack.sql.expression.Alias;
import org.elasticsearch.xpack.sql.expression.Attribute;
import org.elasticsearch.xpack.sql.expression.AttributeSet;
import org.elasticsearch.xpack.sql.expression.Exists;
Expand Down Expand Up @@ -249,8 +250,21 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set<Failure> localFailur
return;
}

// 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(a.groupings(),
if (Expressions.anyMatch(groupingAndMatchingAggregatesAliases,
g -> e.semanticEquals(e instanceof Attribute ? Expressions.attribute(g) : g))) {
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.elasticsearch.xpack.sql.analysis.index.IndexResolution;
import org.elasticsearch.xpack.sql.expression.function.FunctionRegistry;
import org.elasticsearch.xpack.sql.parser.SqlParser;
import org.elasticsearch.xpack.sql.plan.logical.LogicalPlan;
import org.elasticsearch.xpack.sql.type.EsField;
import org.elasticsearch.xpack.sql.type.TypesTests;

Expand All @@ -34,6 +35,13 @@ private String verify(IndexResolution getIndexResult, String sql) {
return e.getMessage().substring(header.length());
}

private LogicalPlan accepted(String sql) {
Map<String, EsField> mapping = TypesTests.loadMapping("mapping-multi-field-with-nested.json");
EsIndex test = new EsIndex("test", mapping);
Analyzer analyzer = new Analyzer(new FunctionRegistry(), IndexResolution.valid(test), TimeZone.getTimeZone("UTC"));
return analyzer.analyze(parser.createStatement(sql), true);
}

public void testMissingIndex() {
assertEquals("1:17: Unknown index [missing]", verify(IndexResolution.notFound("missing"), "SELECT foo FROM missing"));
}
Expand Down Expand Up @@ -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");
assertNotNull(lp);
}

public void testGroupByOrderByScalarOverNonGrouped() {
assertEquals("1:50: Cannot order by non-grouped column [YEAR(date [UTC])], expected [text]",
verify("SELECT MAX(int) FROM test GROUP BY text ORDER BY YEAR(date)"));
Expand Down
2 changes: 2 additions & 0 deletions x-pack/qa/sql/src/main/resources/agg.sql-spec
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ groupByOnNumberWithWhereAndLimit
SELECT emp_no e FROM "test_emp" WHERE emp_no < 10020 GROUP BY emp_no ORDER BY emp_no DESC LIMIT 1;
groupByOnNumberOnAlias
SELECT emp_no e FROM "test_emp" WHERE emp_no < 10020 GROUP BY e ORDER BY emp_no DESC;
groupByOnNumberWithAliasInSelect
SELECT emp_no e FROM "test_emp" WHERE emp_no < 10020 GROUP BY emp_no ORDER BY e DESC;

// group by scalar
groupByAddScalar
Expand Down

0 comments on commit f1d63a7

Please sign in to comment.