Skip to content
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
4a4137f
trying to resolve literals.
codebird May 10, 2019
30a26aa
Merge branch 'master' into issue#41413
codebird May 10, 2019
efb879b
more fixing.
codebird May 11, 2019
7393a3d
fixing the case where a Literal or function is selected but with an a…
codebird May 13, 2019
36087a5
solving 34583 in a rather hacky way
codebird May 14, 2019
710451b
fixing tests.
codebird May 15, 2019
4909761
Merge branch 'master' into issue#41413
codebird May 16, 2019
6702e05
fixing integ tests.
codebird May 17, 2019
b4542cc
Merge branch 'master' into issue#41413
codebird Jun 5, 2019
9cea6c6
Merge branch 'master' into issue#41413
codebird Nov 3, 2019
be4f767
merging and fixing conflicts.
codebird Nov 3, 2019
985f3a5
Merge branch 'master' into issue#41413
codebird Feb 5, 2020
eb95d94
Merging master and updating code after #49570 was merged
codebird Feb 5, 2020
676fd87
Merge remote-tracking branch 'origin/issue#41413' into issue#41413
codebird Feb 5, 2020
863e687
fixing selecting literal with grouping.
codebird Feb 7, 2020
a32a2df
changing based on from @matriv
codebird Feb 7, 2020
cf2f4b0
adding tests, integrity tests, and fixed code after 1 integrity test …
codebird Feb 8, 2020
c6efce8
removing un-needed if.
codebird Feb 9, 2020
374636e
Merge branch 'master' into issue#41413
codebird Feb 9, 2020
d1d640b
merging master, and changes based on @matriv review.
codebird Feb 10, 2020
79e0256
changes based on @matriv review.
codebird Feb 12, 2020
20bdbb6
getting rid of LiteralId, and adding space between if and (
codebird Feb 12, 2020
f40a30f
style fixes.
codebird Feb 13, 2020
92b11bb
Merge branch 'master' into issue#41413
codebird Feb 13, 2020
90de3e5
styling.
codebird Feb 13, 2020
488350a
adding integration test.
codebird Feb 14, 2020
12cb20c
update based on @matriv recommendation.
codebird Feb 14, 2020
196d30b
reverting formatting changes.
codebird Feb 14, 2020
a13cf7e
reverting formatting changes.
codebird Feb 14, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions x-pack/plugin/sql/qa/src/main/resources/agg.sql-spec
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,9 @@ countDistinctAlias
SELECT COUNT(DISTINCT hire_date) AS count FROM test_emp;
countDistinctAndCountSimpleWithAlias
SELECT COUNT(*) cnt, COUNT(DISTINCT first_name) as names, gender FROM test_emp GROUP BY gender ORDER BY gender;
countWithFunctionFilterAndGroupBy
SELECT COUNT(*) as c FROM test_emp WHERE ABS(salary) > 0 GROUP BY gender ORDER BY gender;


aggCountAliasAndWhereClauseMultiGroupBy
SELECT gender g, languages l, COUNT(*) c FROM "test_emp" WHERE emp_no < 10020 GROUP BY gender, languages ORDER BY gender, languages;
Expand Down Expand Up @@ -348,7 +351,7 @@ SELECT gender g, MAX(emp_no) m FROM "test_emp" GROUP BY g HAVING MAX(emp_no) > 1
aggMaxWithHavingOnAlias
SELECT gender g, MAX(emp_no) m FROM "test_emp" GROUP BY g HAVING m > 10 ORDER BY g ;
aggMaxWithMultipleHaving
SELECT gender g, MAX(emp_no) m FROM "test_emp" GROUP BY g HAVING m > 10 AND m < 99999 ORDER BY gender;
SELECT gender g, MAX(emp_no) m FROM "test_emp" GROUP BY g HAVING m > 10 AND m < 99999 ORDER BY gender;
aggMaxWithMultipleHavingBetween
SELECT gender g, MAX(emp_no) m FROM "test_emp" GROUP BY g HAVING m BETWEEN 10 AND 99999 ORDER BY g ;
aggMaxWithMultipleHavingWithLimit
Expand Down Expand Up @@ -435,7 +438,7 @@ SELECT 1 FROM test_emp HAVING COUNT(*) > 0;
implicitGroupingWithLiteralAliasAndFiltering
SELECT 1 AS l FROM test_emp HAVING COUNT(*) > 0;
implicitGroupingWithLiteralAndFilteringOnAlias
SELECT 1, COUNT(*) AS c FROM test_emp HAVING c > 0;
SELECT 1, COUNT(*) AS c FROM test_emp HAVING c > 0;
implicitGroupingWithLiteralAliasAndFilteringOnAlias
SELECT 1 AS l FROM test_emp HAVING COUNT(*) > 0;
implicitGroupingWithAggs
Expand Down Expand Up @@ -497,7 +500,7 @@ SELECT MIN(salary) mi, MAX(salary) ma, MAX(salary) - MIN(salary) AS d FROM test_


//
// Mixture of aggs that get promoted plus filtering on one of them
// Mixture of aggs that get promoted plus filtering on one of them
//
aggMultiWithHaving
SELECT MIN(salary) min, MAX(salary) max, gender g, COUNT(*) c FROM "test_emp" WHERE languages > 0 GROUP BY g HAVING max > 74600 ORDER BY gender;
Expand Down Expand Up @@ -563,5 +566,7 @@ SELECT MIN(salary) min, MAX(salary) max, gender g, languages l, COUNT(*) c FROM
// group by with literal
implicitGroupByWithLiteral
SELECT 10, MAX("salary") FROM test_emp;
literalWithGroupBy
Copy link
Contributor

Choose a reason for hiding this comment

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

Please have the following tests:

  • literalWithGroupBy (withoutAlias)
  • literalsWithGroupBy (withoutAlias)
  • aliasedLiteralWithGroupBy (can have mixed aliased and unaliased)
  • aliasedLiteralsWtihGroupBy (can have mixed aliased and unaliased)
  • literalsWithMultipleGroupBy (aliased and unaliased literals, with more than one column in the GROUP BY which can also be a mix of aliased/unaliased)

SELECT 1 FROM test_emp GROUP BY gender;
groupByWithLiteralAndCount
SELECT 20, COUNT(*) from test_emp GROUP BY gender ORDER BY 2;
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.elasticsearch.xpack.ql.expression.Expression;
import org.elasticsearch.xpack.ql.expression.Expressions;
import org.elasticsearch.xpack.ql.expression.FieldAttribute;
import org.elasticsearch.xpack.ql.expression.Literal;
import org.elasticsearch.xpack.ql.expression.NamedExpression;
import org.elasticsearch.xpack.ql.expression.Order;
import org.elasticsearch.xpack.ql.expression.ReferenceAttribute;
Expand Down Expand Up @@ -70,6 +71,7 @@
import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;
import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toSet;
import static org.elasticsearch.xpack.ql.util.CollectionUtils.combine;

public class Analyzer extends RuleExecutor<LogicalPlan> {
Expand Down Expand Up @@ -341,6 +343,22 @@ else if (plan instanceof Aggregate) {
List<Expression> groupings = a.groupings();
List<Expression> newGroupings = new ArrayList<>();
AttributeMap<Expression> resolved = Expressions.aliases(a.aggregates());

//if all we're selecting are functions or literals, even if we're giving them aliases.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//if all we're selecting are functions or literals, even if we're giving them aliases.
// if all we're selecting are functions or literals, even if we're giving them aliases.

final boolean[] updateResolved = {true};
a.forEachUp(p -> p.forEachExpressionsUp(e -> {
if (e instanceof Alias
&& ((Alias) e).child() instanceof Function == false
&& ((Alias) e).child() instanceof Literal == false) {
updateResolved[0] = false;
}
}));
if(updateResolved[0]){
var allFields = plan.inputSet().stream().map(NamedExpression.class::cast)
.collect(toList());
allFields.addAll(a.aggregates());
resolved = Expressions.asAttributeMap(allFields);
}
boolean changed = false;
for (Expression grouping : groupings) {
if (grouping instanceof UnresolvedAttribute) {
Expand Down Expand Up @@ -618,7 +636,7 @@ protected LogicalPlan rule(LogicalPlan plan) {
for (Order or : o.order()) {
maybeResolved.add(or.resolved() ? or : tryResolveExpression(or, child));
}

Stream<Order> referencesStream = maybeResolved.stream()
.filter(Expression::resolved);

Expand All @@ -629,7 +647,7 @@ protected LogicalPlan rule(LogicalPlan plan) {
// a + 1 in SELECT is actually Alias("a + 1", a + 1) and translates to ReferenceAttribute
// in the output. However it won't match the unnamed a + 1 despite being the same expression
// so explicitly compare the source

// if there's a match, remove the item from the reference stream
if (Expressions.hasReferenceAttribute(child.outputSet())) {
final Map<Attribute, Expression> collectRefs = new LinkedHashMap<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@
import java.util.Set;
import java.util.function.Consumer;

import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;
import static org.elasticsearch.xpack.ql.expression.Expressions.equalsAsAttribute;
import static org.elasticsearch.xpack.ql.expression.Literal.FALSE;
Expand Down Expand Up @@ -2112,7 +2113,7 @@ protected LogicalPlan rule(LogicalPlan plan) {

plan.forEachDown(a -> {
List<Object> values = extractConstants(a.aggregates());
if (values.size() == a.aggregates().size() && isNotQueryWithFromClauseAndFilterFoldedToFalse(a)) {
if (values.size() == a.aggregates().size() && a.groupings().isEmpty() && isNotQueryWithFromClauseAndFilterFoldedToFalse(a)) {
optimizedPlan.set(new LocalRelation(a.source(), new SingletonExecutable(a.output(), values.toArray())));
}
}, Aggregate.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -392,11 +392,11 @@ protected PhysicalPlan rule(AggregateExec a) {
}
return a;
}

static EsQueryExec fold(AggregateExec a, EsQueryExec exec) {

QueryContainer queryC = exec.queryContainer();

// track aliases defined in the SELECT and used inside GROUP BY
// SELECT x AS a ... GROUP BY a
Map<Attribute, Expression> aliasMap = new LinkedHashMap<>();
Expand All @@ -405,7 +405,7 @@ static EsQueryExec fold(AggregateExec a, EsQueryExec exec) {
aliasMap.put(ne.toAttribute(), ((Alias) ne).child());
}
}

if (aliasMap.isEmpty() == false) {
Map<Attribute, Expression> newAliases = new LinkedHashMap<>(queryC.aliases());
newAliases.putAll(aliasMap);
Expand Down Expand Up @@ -455,6 +455,13 @@ static EsQueryExec fold(AggregateExec a, EsQueryExec exec) {
// literal
if (target.foldable()) {
queryC = queryC.addColumn(ne.toAttribute());
//If we're only selecting literals, we have to fix the row count
if(a.aggregates().stream().allMatch(s -> s.children().get(0) instanceof Literal)) {
for (Expression grouping : a.groupings()) {
GroupByKey matchingGroup = groupingContext.groupFor(grouping);
queryC = queryC.addColumn(new GroupByRef(matchingGroup.id(), null, false), id);
}
}
}

// look at functions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@
import org.elasticsearch.xpack.ql.expression.Expression;
import org.elasticsearch.xpack.ql.expression.FieldAttribute;
import org.elasticsearch.xpack.ql.expression.Literal;
import org.elasticsearch.xpack.ql.expression.NamedExpression;
import org.elasticsearch.xpack.ql.expression.function.aggregate.Count;
import org.elasticsearch.xpack.ql.expression.gen.script.ScriptTemplate;
import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.GreaterThan;
import org.elasticsearch.xpack.ql.index.EsIndex;
import org.elasticsearch.xpack.ql.index.IndexResolution;
import org.elasticsearch.xpack.ql.plan.logical.Aggregate;
Expand Down Expand Up @@ -130,6 +133,29 @@ public void testTermEqualityAnalyzer() {
assertEquals("value", tq.value());
}

public void testAliasAndGroupByResolution(){
LogicalPlan p = plan("SELECT COUNT(*) AS c FROM test WHERE ABS(int) > 0 GROUP BY int");
assertTrue(p instanceof Aggregate);
var pc = ((Aggregate) p).child();
assertTrue(pc instanceof Filter);
Expression condition = ((Filter) pc).condition();
assertEquals(((GreaterThan) condition).functionName(), "GREATERTHAN");
List<Expression> groupings = ((Aggregate) p).groupings();
assertTrue(groupings.get(0).resolved());
var agg = ((Aggregate) p).aggregates();
assertEquals((agg.get(0)).name(), "c");
assertEquals(((Count) ((Alias) agg.get(0)).child()).functionName(), "COUNT");
}
public void testLiteralWithGroupBy(){
LogicalPlan p = plan("SELECT 1 as t FROM test GROUP BY int");
assertTrue(p instanceof Aggregate);
List<Expression> groupings = ((Aggregate) p).groupings();
assertTrue(groupings.get(0).resolved());
var agg = ((Aggregate) p).aggregates();
assertEquals((agg.get(0)).name(), "t");
assertTrue(((Alias) agg.get(0)).child() instanceof Literal);
}

public void testTermEqualityNotAnalyzed() {
LogicalPlan p = plan("SELECT some.string FROM test WHERE int = 5");
assertTrue(p instanceof Project);
Expand Down