From f37c93e4ce621077923a9076e2bc0de5caf7edec Mon Sep 17 00:00:00 2001 From: Andras Palinkas Date: Thu, 11 Mar 2021 16:57:31 -0500 Subject: [PATCH] SQL: Resolve attributes recursively for improved subquery support (#69765) (#70325) Previously we did not resolve the attributes recursively which meant that if a field or expression was re-aliased multiple times (through multiple levels of subqueries), the aliases were only resolved one level down. This led to failed query translation because `ReferenceAttribute`s were pointing to non-existing attributes during query translation. For example the query ```sql SELECT i AS j FROM ( SELECT int AS i FROM test) ORDER BY j ``` failed during translation because the `OrderBy` resolved the `j` ReferenceAttribute to another `i` ReferenceAttribute that was later removed by an Optimization: ``` OrderBy[[Order[j{r}#4,ASC,LAST]]] ! OrderBy[[Order[i{r}#2,ASC,LAST]]] \_Project[[j]] = \_Project[[j]] \_Project[[i]] ! \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] ! ``` By resolving the `Attributes` recursively both `j{r}` and `i{r}` will resolve to `test.int{f}` above: ``` OrderBy[[Order[test.int{f}#22,ASC,LAST]]] = OrderBy[[Order[test.int{f}#22,ASC,LAST]]] \_Project[[j]] = \_Project[[j]] \_Project[[i]] ! \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] \_EsRelation[test][date{f}#6, some{f}#7, some.string{f}#8, some.string..] ! ``` The scope of recursive resolution depends on how the `AttributeMap` is constructed and populated. Fixes #67237 --- .../xpack/ql/expression/AttributeMap.java | 38 ++++++++++-- .../ql/expression/AttributeMapTests.java | 61 +++++++++++++++++++ .../src/main/resources/subselect.sql-spec | 28 ++++++++- .../xpack/sql/analysis/analyzer/Analyzer.java | 4 +- .../xpack/sql/analysis/analyzer/Verifier.java | 29 +++++---- .../xpack/sql/optimizer/Optimizer.java | 8 +-- .../xpack/sql/planner/QueryFolder.java | 7 ++- .../querydsl/container/QueryContainer.java | 14 +++-- .../analyzer/VerifierErrorMessagesTests.java | 11 +++- .../sql/planner/QueryTranslatorTests.java | 47 +++++++++++++- 10 files changed, 211 insertions(+), 36 deletions(-) diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java index 36a2a16b47b33..cffde512b669f 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/AttributeMap.java @@ -6,6 +6,8 @@ */ package org.elasticsearch.xpack.ql.expression; +import org.elasticsearch.xpack.ql.QlIllegalArgumentException; + import java.util.AbstractSet; import java.util.Collection; import java.util.Iterator; @@ -148,6 +150,8 @@ public static final AttributeMap emptyAttributeMap() { return EMPTY; } + private static final Object NOT_FOUND = new Object(); + private final Map delegate; private Set keySet = null; private Collection values = null; @@ -238,14 +242,14 @@ public boolean isEmpty() { @Override public boolean containsKey(Object key) { if (key instanceof NamedExpression) { - return delegate.keySet().contains(new AttributeWrapper(((NamedExpression) key).toAttribute())); + return delegate.containsKey(new AttributeWrapper(((NamedExpression) key).toAttribute())); } return false; } @Override public boolean containsValue(Object value) { - return delegate.values().contains(value); + return delegate.containsValue(value); } @Override @@ -258,10 +262,32 @@ public E get(Object key) { @Override public E getOrDefault(Object key, E defaultValue) { - E e; - return (((e = get(key)) != null) || containsKey(key)) - ? e - : defaultValue; + if (key instanceof NamedExpression) { + return delegate.getOrDefault(new AttributeWrapper(((NamedExpression) key).toAttribute()), defaultValue); + } + return defaultValue; + } + + public E resolve(Object key) { + return resolve(key, null); + } + + public E resolve(Object key, E defaultValue) { + E value = defaultValue; + E candidate = null; + int allowedLookups = 1000; + while ((candidate = get(key)) != null || containsKey(key)) { + // instead of circling around, return + if (candidate == key) { + return candidate; + } + if (--allowedLookups == 0) { + throw new QlIllegalArgumentException("Potential cycle detected"); + } + key = candidate; + value = candidate; + } + return value; } @Override diff --git a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/AttributeMapTests.java b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/AttributeMapTests.java index 669bf5a35b994..c12135c8da9e3 100644 --- a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/AttributeMapTests.java +++ b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/AttributeMapTests.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.ql.expression; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.ql.QlIllegalArgumentException; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataTypes; @@ -18,6 +19,8 @@ import java.util.Set; import static java.util.stream.Collectors.toList; +import static org.elasticsearch.xpack.ql.TestUtils.fieldAttribute; +import static org.elasticsearch.xpack.ql.TestUtils.of; import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.arrayWithSize; import static org.hamcrest.Matchers.contains; @@ -62,6 +65,64 @@ public void testAttributeMapWithSameAliasesCanResolveAttributes() { assertTrue(newAttributeMap.get(param2.toAttribute()) == param2.child()); } + public void testResolve() { + AttributeMap.Builder builder = AttributeMap.builder(); + Attribute one = a("one"); + Attribute two = fieldAttribute("two", DataTypes.INTEGER); + Attribute three = fieldAttribute("three", DataTypes.INTEGER); + Alias threeAlias = new Alias(Source.EMPTY, "three_alias", three); + Alias threeAliasAlias = new Alias(Source.EMPTY, "three_alias_alias", threeAlias); + builder.put(one, of("one")); + builder.put(two, "two"); + builder.put(three, of("three")); + builder.put(threeAlias.toAttribute(), threeAlias.child()); + builder.put(threeAliasAlias.toAttribute(), threeAliasAlias.child()); + AttributeMap map = builder.build(); + + assertEquals(of("one"), map.resolve(one)); + assertEquals("two", map.resolve(two)); + assertEquals(of("three"), map.resolve(three)); + assertEquals(of("three"), map.resolve(threeAlias)); + assertEquals(of("three"), map.resolve(threeAliasAlias)); + assertEquals(of("three"), map.resolve(threeAliasAlias, threeAlias)); + Attribute four = a("four"); + assertEquals("not found", map.resolve(four, "not found")); + assertNull(map.resolve(four)); + assertEquals(four, map.resolve(four, four)); + } + + public void testResolveOneHopCycle() { + AttributeMap.Builder builder = AttributeMap.builder(); + Attribute a = fieldAttribute("a", DataTypes.INTEGER); + Attribute b = fieldAttribute("b", DataTypes.INTEGER); + builder.put(a, a); + builder.put(b, a); + AttributeMap map = builder.build(); + + assertEquals(a, map.resolve(a, "default")); + assertEquals(a, map.resolve(b, "default")); + assertEquals("default", map.resolve("non-existing-key", "default")); + } + + public void testResolveMultiHopCycle() { + AttributeMap.Builder builder = AttributeMap.builder(); + Attribute a = fieldAttribute("a", DataTypes.INTEGER); + Attribute b = fieldAttribute("b", DataTypes.INTEGER); + Attribute c = fieldAttribute("c", DataTypes.INTEGER); + Attribute d = fieldAttribute("d", DataTypes.INTEGER); + builder.put(a, b); + builder.put(b, c); + builder.put(c, d); + builder.put(d, a); + AttributeMap map = builder.build(); + + // note: multi hop cycles should not happen, unless we have a + // bug in the code that populates the AttributeMaps + expectThrows(QlIllegalArgumentException.class, () -> { + assertEquals(a, map.resolve(a, c)); + }); + } + private Alias createIntParameterAlias(int index, int value) { Source source = new Source(1, index * 5, "?"); Literal literal = new Literal(source, value, DataTypes.INTEGER); diff --git a/x-pack/plugin/sql/qa/server/src/main/resources/subselect.sql-spec b/x-pack/plugin/sql/qa/server/src/main/resources/subselect.sql-spec index c1ee832013696..53cd6cc38d7ef 100644 --- a/x-pack/plugin/sql/qa/server/src/main/resources/subselect.sql-spec +++ b/x-pack/plugin/sql/qa/server/src/main/resources/subselect.sql-spec @@ -16,10 +16,34 @@ basicGroupBy SELECT gender FROM (SELECT first_name AS f, last_name, gender FROM test_emp) GROUP BY gender ORDER BY gender ASC; basicGroupByAlias SELECT g FROM (SELECT first_name AS f, last_name, gender AS g FROM test_emp) GROUP BY g ORDER BY g ASC; -// @AwaitsFix(bugUrl = "follow-up to https://github.com/elastic/elasticsearch/pull/67216") -basicGroupByWithFilterByAlias-Ignore +basicGroupByWithFilterByAlias SELECT g FROM (SELECT first_name AS f, last_name, gender AS g FROM test_emp) WHERE g IS NOT NULL GROUP BY g ORDER BY g ASC; +basicGroupByRealiased +SELECT g AS h FROM (SELECT first_name AS f, last_name, gender AS g FROM test_emp) GROUP BY g ORDER BY g DESC NULLS last; +basicGroupByRealiasedTwice +SELECT g AS h FROM (SELECT first_name AS f, last_name, gender AS g FROM test_emp) GROUP BY h ORDER BY h DESC NULLS last; +basicOrderByRealiasedField +SELECT g AS h FROM (SELECT first_name AS f, last_name, gender AS g FROM test_emp) ORDER BY g DESC NULLS first; + +groupAndOrderByRealiasedExpression +SELECT emp_group AS e, min_high_salary AS s +FROM ( + SELECT emp_no % 2 AS emp_group, MIN(salary) AS min_high_salary + FROM test_emp + WHERE salary > 50000 + GROUP BY emp_group +) +ORDER BY e DESC; + +multiLevelSelectStar +SELECT * FROM (SELECT * FROM ( SELECT * FROM test_emp )); + +multiLevelSelectStarWithAlias +SELECT * FROM (SELECT * FROM ( SELECT * FROM test_emp ) b) c; +// AwaitsFix: https://github.com/elastic/elasticsearch/issues/69758 +filterAfterGroupBy-Ignore +SELECT s2 AS s3 FROM (SELECT s AS s2 FROM ( SELECT salary AS s FROM test_emp) GROUP BY s2) WHERE s2 < 5 ORDER BY s3 DESC NULLS last; countAndComplexCondition SELECT COUNT(*) as c FROM (SELECT * FROM test_emp WHERE gender IS NOT NULL) WHERE ABS(salary) > 0 GROUP BY gender ORDER BY gender; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java index de480b2c4b261..e4044f0c06738 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java @@ -638,12 +638,12 @@ protected LogicalPlan doRule(LogicalPlan plan) { AttributeMap.Builder builder = AttributeMap.builder(); // collect aliases child.forEachUp(p -> p.forEachExpressionUp(Alias.class, a -> builder.put(a.toAttribute(), a.child()))); - final Map collectRefs = builder.build(); + final AttributeMap collectRefs = builder.build(); referencesStream = referencesStream.filter(r -> { for (Attribute attr : child.outputSet()) { if (attr instanceof ReferenceAttribute) { - Expression source = collectRefs.getOrDefault(attr, attr); + Expression source = collectRefs.resolve(attr, attr); // found a match, no need to resolve it further // so filter it out if (source.equals(r.child())) { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index ce2759deb7a36..3fbb058df6c3d 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -316,10 +316,11 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur Map> missing = new LinkedHashMap<>(); o.order().forEach(oe -> { - Expression e = oe.child(); + final Expression e = oe.child(); + final Expression resolvedE = attributeRefs.resolve(e, e); // aggregates are allowed - if (Functions.isAggregate(attributeRefs.getOrDefault(e, e))) { + if (Functions.isAggregate(resolvedE)) { return; } @@ -340,8 +341,12 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur // 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)))) { + if (resolvedE.anyMatch(expression -> Expressions.anyMatch(groupingAndMatchingAggregatesAliases, + g -> { + Expression resolvedG = attributeRefs.resolve(g, g); + resolvedG = expression instanceof Attribute ? Expressions.attribute(resolvedG) : resolvedG; + return expression.semanticEquals(resolvedG); + }))) { return; } @@ -406,7 +411,7 @@ private static boolean checkGroupByHavingHasOnlyAggs(Expression e, Set expressions, expressions.forEach(e -> e.forEachDown(c -> { if (c instanceof ReferenceAttribute) { - c = attributeRefs.getOrDefault(c, c); + c = attributeRefs.resolve(c, c); } if (c instanceof Function) { localFailures.add(fail(c, "No functions allowed (yet); encountered [{}]", c.sourceText())); @@ -579,7 +584,7 @@ private static boolean checkGroupMatch(Expression e, Node source, List localFailures, LogicalPlan filterChild = filter.child(); if (filterChild instanceof Aggregate == false) { filter.condition().forEachDown(Expression.class, e -> { - if (Functions.isAggregate(attributeRefs.getOrDefault(e, e))) { + if (Functions.isAggregate(attributeRefs.resolve(e, e))) { if (filterChild instanceof Project) { filter.condition().forEachDown(FieldAttribute.class, f -> localFailures.add(fail(e, "[{}] field must appear in the GROUP BY clause or in an aggregate function", @@ -690,7 +695,7 @@ private static void checkFilterOnGrouping(LogicalPlan p, Set localFailu if (p instanceof Filter) { Filter filter = (Filter) p; filter.condition().forEachDown(Expression.class, e -> { - if (Functions.isGrouping(attributeRefs.getOrDefault(e, e))) { + if (Functions.isGrouping(attributeRefs.resolve(e, e))) { localFailures .add(fail(e, "Cannot filter on grouping function [{}], use its argument instead", Expressions.name(e))); } @@ -717,7 +722,7 @@ private static void checkNestedUsedInGroupByOrHavingOrWhereOrOrderBy(LogicalPlan } }; Consumer checkForNested = e -> - attributeRefs.getOrDefault(e, e).forEachUp(FieldAttribute.class, matchNested); + attributeRefs.resolve(e, e).forEachUp(FieldAttribute.class, matchNested); Consumer checkForNestedInFunction = f -> f.arguments().forEach( arg -> arg.forEachUp(FieldAttribute.class, matchNested)); @@ -739,7 +744,7 @@ private static void checkNestedUsedInGroupByOrHavingOrWhereOrOrderBy(LogicalPlan // check in where (scalars not allowed) p.forEachDown(Filter.class, f -> f.condition().forEachUp(e -> - attributeRefs.getOrDefault(e, e).forEachUp(ScalarFunction.class, sf -> { + attributeRefs.resolve(e, e).forEachUp(ScalarFunction.class, sf -> { if (sf instanceof BinaryComparison == false && sf instanceof IsNull == false && sf instanceof IsNotNull == false && @@ -758,7 +763,7 @@ private static void checkNestedUsedInGroupByOrHavingOrWhereOrOrderBy(LogicalPlan // check in order by (scalars not allowed) p.forEachDown(OrderBy.class, ob -> ob.order().forEach(o -> o.forEachUp(e -> - attributeRefs.getOrDefault(e, e).forEachUp(ScalarFunction.class, checkForNestedInFunction) + attributeRefs.resolve(e, e).forEachUp(ScalarFunction.class, checkForNestedInFunction) ))); if (nested.isEmpty() == false) { localFailures.add( diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java index 9942d1df31cc9..d7658697d228f 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java @@ -222,8 +222,8 @@ public LogicalPlan apply(LogicalPlan plan) { AttributeMap.Builder builder = AttributeMap.builder(); // collect aliases plan.forEachExpressionUp(Alias.class, a -> builder.put(a.toAttribute(), a.child())); - final Map collectRefs = builder.build(); - java.util.function.Function replaceReference = r -> collectRefs.getOrDefault(r, r); + final AttributeMap collectRefs = builder.build(); + java.util.function.Function replaceReference = r -> collectRefs.resolve(r, r); plan = plan.transformUp(p -> { // non attribute defining plans get their references removed @@ -300,7 +300,7 @@ static class PruneOrderByNestedFields extends OptimizerRule { private void findNested(Expression exp, AttributeMap functions, Consumer onFind) { exp.forEachUp(e -> { if (e instanceof ReferenceAttribute) { - Function f = functions.get(e); + Function f = functions.resolve(e); if (f != null) { findNested(f, functions, onFind); } @@ -578,7 +578,7 @@ private List combineProjections(List // replace any matching attribute with a lower alias (if there's a match) // but clean-up non-top aliases at the end for (NamedExpression ne : upper) { - NamedExpression replacedExp = (NamedExpression) ne.transformUp(Attribute.class, a -> aliases.getOrDefault(a, a)); + NamedExpression replacedExp = (NamedExpression) ne.transformUp(Attribute.class, a -> aliases.resolve(a, a)); replaced.add((NamedExpression) CleanAliases.trimNonTopLevelAliases(replacedExp)); } return replaced; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java index 46bd7fe17a1c6..aa8cf9f2d66e9 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java @@ -600,10 +600,13 @@ else if (target.foldable()) { else { GroupByKey matchingGroup = null; if (groupingContext != null) { + target = queryC.aliases().resolve(target, target); + id = Expressions.id(target); matchingGroup = groupingContext.groupFor(target); Check.notNull(matchingGroup, "Cannot find group [{}]", Expressions.name(ne)); - queryC = queryC.addColumn(new GroupByRef(matchingGroup.id(), null, isDateBased(ne.dataType())), id); + queryC = queryC.addColumn( + new GroupByRef(matchingGroup.id(), null, isDateBased(ne.dataType())), id); } // fallback else { @@ -707,7 +710,7 @@ protected PhysicalPlan rule(OrderExec plan) { // if it's a reference, get the target expression if (orderExpression instanceof ReferenceAttribute) { - orderExpression = qContainer.aliases().get(orderExpression); + orderExpression = qContainer.aliases().resolve(orderExpression); } String lookup = Expressions.id(orderExpression); GroupByKey group = qContainer.findGroupForAgg(lookup); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java index e0b1d081ec322..840455c54d166 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java @@ -77,7 +77,7 @@ public class QueryContainer { // scalar function processors - recorded as functions get folded; // at scrolling, their inputs (leaves) get updated - private final AttributeMap scalarFunctions; + private AttributeMap scalarFunctions; private final Map sort; private final int limit; @@ -185,7 +185,7 @@ public BitSet columnMask(List columns) { aliasName(columns.get(0)); for (Attribute column : columns) { - Expression expression = aliases.getOrDefault(column, column); + Expression expression = aliases.resolve(column, column); // find the column index String id = Expressions.id(expression); @@ -375,7 +375,11 @@ static Query rewriteToContainNestedField(@Nullable Query query, Source source, S // replace function/operators's input with references private Tuple resolvedTreeComputingRef(ScalarFunction function, Attribute attr) { - Pipe proc = scalarFunctions.computeIfAbsent(attr, v -> function.asPipe()); + Pipe proc = null; + if ((proc = scalarFunctions.resolve(attr)) == null) { + proc = function.asPipe(); + scalarFunctions = AttributeMap.builder(scalarFunctions).put(attr, proc).build(); + } // find the processor inputs (Attributes) and convert them into references // no need to promote them to the top since the container doesn't have to be aware @@ -407,14 +411,14 @@ public FieldExtraction resolve(Attribute attribute) { } public QueryContainer addColumn(Attribute attr) { - Expression expression = aliases.getOrDefault(attr, attr); + Expression expression = aliases.resolve(attr, attr); Tuple tuple = asFieldExtraction(attr); return tuple.v1().addColumn(tuple.v2(), Expressions.id(expression)); } private Tuple asFieldExtraction(Attribute attr) { // resolve it Expression - Expression expression = aliases.getOrDefault(attr, attr); + Expression expression = aliases.resolve(attr, attr); if (expression instanceof FieldAttribute) { FieldAttribute fa = (FieldAttribute) expression; diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index 27c6736f5ef86..235c16ffdd3d7 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -506,6 +506,8 @@ public void testGroupByOrderByScalarOverNonGrouped() { public void testGroupByOrderByFieldFromGroupByFunction() { assertEquals("1:54: Cannot order by non-grouped column [int], expected [ABS(int)]", error("SELECT ABS(int) FROM test GROUP BY ABS(int) ORDER BY int")); + assertEquals("1:91: Cannot order by non-grouped column [c], expected [b] or an aggregate function", + error("SELECT b, abs, 2 as c FROM (SELECT bool as b, ABS(int) abs FROM test) GROUP BY b ORDER BY c")); } public void testGroupByOrderByScalarOverNonGrouped_WithHaving() { @@ -515,7 +517,14 @@ public void testGroupByOrderByScalarOverNonGrouped_WithHaving() { public void testGroupByHavingNonGrouped() { assertEquals("1:48: Cannot use HAVING filter on non-aggregate [int]; use WHERE instead", - error("SELECT AVG(int) FROM test GROUP BY text HAVING int > 10")); + error("SELECT AVG(int) FROM test GROUP BY bool HAVING int > 10")); + accept("SELECT AVG(int) FROM test GROUP BY bool HAVING AVG(int) > 2"); + } + + @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/69758") + public void testGroupByWhereSubselect() { + accept("SELECT b, a FROM (SELECT bool as b, AVG(int) as a FROM test GROUP BY bool) WHERE b = false"); + accept("SELECT b, a FROM (SELECT bool as b, AVG(int) as a FROM test GROUP BY bool HAVING AVG(int) > 2) WHERE b = false"); } public void testGroupByAggregate() { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index 7fa3f2f16983c..34c7ea5549270 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java @@ -2558,8 +2558,7 @@ public void testSubqueryFilterOrderByAlias() throws Exception { "WHERE i IS NOT NULL " + "ORDER BY i"); } - - @AwaitsFix(bugUrl = "follow-up to https://github.com/elastic/elasticsearch/pull/67216") + public void testSubqueryGroupByFilterAndOrderByByAlias() throws Exception { PhysicalPlan p = optimizeAndPlan("SELECT i FROM " + "( SELECT int AS i FROM test ) " + @@ -2617,4 +2616,48 @@ public void testSubqueryWithAliasOrderByAlias() throws Exception { "( SELECT int AS i FROM test ) AS s " + "ORDER BY s.i > 10"); } + + public void testMultiLevelSubqueriesOrderByByAlias() { + optimizeAndPlan("SELECT i AS j FROM ( SELECT int AS i FROM test) ORDER BY j"); + optimizeAndPlan("SELECT j AS k FROM (SELECT i AS j FROM ( SELECT int AS i FROM test)) ORDER BY k"); + } + + public void testSubqueryGroupByOrderByAliasedExpression() { + optimizeAndPlan("SELECT int_group AS g, min_date AS d " + + "FROM (" + + " SELECT int % 2 AS int_group, MIN(date) AS min_date " + + " FROM test WHERE date > '1970-01-01'::datetime " + + " GROUP BY int_group" + + ") " + + "ORDER BY d DESC"); + } + + public void testSubqueryGroupByOrderByAliasedAggFunction() { + optimizeAndPlan("SELECT int_group AS g, min_date AS d " + + "FROM (" + + " SELECT int % 2 AS int_group, MIN(date) AS min_date " + + " FROM test WHERE date > '1970-01-01'::datetime " + + " GROUP BY int_group " + + ")" + + "ORDER BY g DESC"); + } + + public void testMultiLevelSubquerySelectStar() { + optimizeAndPlan("SELECT * FROM (SELECT * FROM ( SELECT * FROM test ))"); + optimizeAndPlan("SELECT * FROM (SELECT * FROM ( SELECT * FROM test ) b) c"); + } + + public void testMultiLevelSubqueryGroupBy() { + optimizeAndPlan("SELECT i AS j FROM ( SELECT int AS i FROM test) GROUP BY j"); + optimizeAndPlan("SELECT j AS k FROM (SELECT i AS j FROM ( SELECT int AS i FROM test)) GROUP BY k"); + } + + public void testSubqueryGroupByFilterAndOrderByByRealiased() { + optimizeAndPlan("SELECT g as h FROM (SELECT date AS f, int AS g FROM test) WHERE h IS NOT NULL GROUP BY h ORDER BY h ASC"); + } + + @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/69758") + public void testFilterAfterGroupBy() { + optimizeAndPlan("SELECT j AS k FROM (SELECT i AS j FROM ( SELECT int AS i FROM test) GROUP BY j) WHERE j < 5"); + } }