From aba2f6313b4af3faf8d86bb750f32704a983306f Mon Sep 17 00:00:00 2001 From: Andras Palinkas Date: Mon, 1 Mar 2021 20:30:07 -0500 Subject: [PATCH 1/5] Resolve references recursively Code simplification in the AttributeMap --- .../xpack/ql/expression/AttributeMap.java | 42 ++++++++++++++----- .../ql/expression/AttributeMapTests.java | 31 ++++++++++++++ .../src/main/resources/subselect.sql-spec | 22 +++++++++- .../xpack/sql/analysis/analyzer/Verifier.java | 5 ++- .../xpack/sql/optimizer/Optimizer.java | 6 +-- .../xpack/sql/planner/QueryFolder.java | 6 ++- .../analyzer/VerifierErrorMessagesTests.java | 9 +++- .../sql/planner/QueryTranslatorTests.java | 28 ++++++++++++- 8 files changed, 127 insertions(+), 22 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 b976238130466..985ed6027c8fa 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 @@ -238,30 +238,52 @@ 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); } + /** + * @param key {@link NamedExpression} to look up + * @return Looks up the key in the AttributeMap recursively, meaning if the value for this key + * is another key in the map, it will follow it. It will return the final value or null if the key + * does not exist in the map. + */ @Override public E get(Object key) { - if (key instanceof NamedExpression) { - return delegate.get(new AttributeWrapper(((NamedExpression) key).toAttribute())); - } - return null; + return getOrDefault(key, null); } + /** + * @param key {@link NamedExpression} to look up + * @return Looks up the key in the AttributeMap recursively, meaning if the value for this key + * is another key in the map, it will follow it. It will return the final value or the + * specified default value if the key does not exist in the map. + */ @Override public E getOrDefault(Object key, E defaultValue) { - E e; - return (((e = get(key)) != null) || containsKey(key)) - ? e - : defaultValue; + E candidate = defaultValue; + E value = null; + while (((value = lookup(key)) != null || containsKey(key)) && value != key) { + key = candidate = value; + } + return candidate; + } + + /** + * @param key {@link NamedExpression} to look up + * @return Result of the non-recursive lookup of the key in the map. + */ + private E lookup(Object key) { + if (key instanceof NamedExpression) { + return delegate.get(new AttributeWrapper(((NamedExpression) key).toAttribute())); + } + return null; } @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 1ec93d2436526..85352b906a0ff 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 @@ -17,6 +17,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; @@ -61,6 +63,35 @@ public void testAttributeMapWithSameAliasesCanResolveAttributes() { assertTrue(newAttributeMap.get(param2.toAttribute()) == param2.child()); } + public void testResolveRecursively() { + 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.get(one)); + assertEquals(map.get(one), map.getOrDefault(one, null)); + assertEquals("two", map.get(two)); + assertEquals(map.get(two), map.getOrDefault(two, null)); + assertEquals(of("three"), map.get(three)); + assertEquals(map.get(three), map.getOrDefault(three, null)); + assertEquals(map.get(three), map.getOrDefault(threeAlias, null)); + assertEquals(map.get(three), map.get(threeAlias)); + assertEquals(map.get(three), map.getOrDefault(threeAliasAlias, null)); + assertEquals(map.get(three), map.get(threeAliasAlias)); + Attribute four = a("four"); + assertEquals("not found", map.getOrDefault(four, "not found")); + assertNull(map.get(four)); + } + 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..2b346ad6fc6cd 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,28 @@ 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; +// 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/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index ce2759deb7a36..5ff9fdc58b0b9 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 @@ -317,9 +317,10 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur o.order().forEach(oe -> { Expression e = oe.child(); + e = attributeRefs.getOrDefault(e, e); // aggregates are allowed - if (Functions.isAggregate(attributeRefs.getOrDefault(e, e))) { + if (Functions.isAggregate(e)) { return; } @@ -341,7 +342,7 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur // // Also, make sure to compare attributes directly if (e.anyMatch(expression -> Expressions.anyMatch(groupingAndMatchingAggregatesAliases, - g -> expression.semanticEquals(expression instanceof Attribute ? Expressions.attribute(g) : g)))) { + g -> expression.semanticEquals(attributeRefs.getOrDefault(Expressions.attribute(g), g))))) { return; } 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..9678bcb38e9ae 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 @@ -131,8 +131,8 @@ protected Iterable.Batch> batches() { ); Batch refs = new Batch("Replace References", Limiter.ONCE, - new ReplaceReferenceAttributeWithSource() - ); + new ReplaceReferenceAttributeWithSource() + ); Batch operators = new Batch("Operator Optimization", // combining @@ -222,7 +222,7 @@ 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(); + final AttributeMap collectRefs = builder.build(); java.util.function.Function replaceReference = r -> collectRefs.getOrDefault(r, r); plan = plan.transformUp(p -> { 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..02f9a02e061a8 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,12 @@ else if (target.foldable()) { else { GroupByKey matchingGroup = null; if (groupingContext != null) { - matchingGroup = groupingContext.groupFor(target); + final Expression resolvedTarget = queryC.aliases().getOrDefault(target, target); + matchingGroup = groupingContext.groupFor(resolvedTarget); 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())), Expressions.id(resolvedTarget)); } // fallback else { 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 c3df2b11e222f..bde3494dc7e77 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 @@ -517,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 7365e6dd33c9e..af1593e22c637 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 @@ -2599,8 +2599,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 ) " + @@ -2658,4 +2657,29 @@ public void testSubqueryWithAliasOrderByAlias() throws Exception { "( SELECT int AS i FROM test ) AS s " + "ORDER BY s.i > 10"); } + + public void testReferenceResolutionInSubqueries() { + 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"); + 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"); + 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"); + 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"); + optimizeAndPlan("SELECT g FROM (SELECT date AS f, int AS g FROM test) WHERE g IS NOT NULL GROUP BY g ORDER BY g 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"); + } } From 015acb36ed1f289228226dd04594c2d47608a046 Mon Sep 17 00:00:00 2001 From: Andras Palinkas Date: Tue, 2 Mar 2021 18:22:17 -0500 Subject: [PATCH 2/5] Verifier error message fix --- .../xpack/sql/analysis/analyzer/Verifier.java | 14 +++++++++----- .../analyzer/VerifierErrorMessagesTests.java | 2 ++ 2 files changed, 11 insertions(+), 5 deletions(-) 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 5ff9fdc58b0b9..b86f519ab57ff 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,11 +316,11 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur Map> missing = new LinkedHashMap<>(); o.order().forEach(oe -> { - Expression e = oe.child(); - e = attributeRefs.getOrDefault(e, e); + final Expression e = oe.child(); + final Expression resolvedE = attributeRefs.getOrDefault(e, e); // aggregates are allowed - if (Functions.isAggregate(e)) { + if (Functions.isAggregate(resolvedE)) { return; } @@ -341,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(attributeRefs.getOrDefault(Expressions.attribute(g), g))))) { + if (resolvedE.anyMatch(expression -> Expressions.anyMatch(groupingAndMatchingAggregatesAliases, + g -> { + Expression resolvedG = attributeRefs.getOrDefault(g, g); + resolvedG = expression instanceof Attribute ? Expressions.attribute(resolvedG) : resolvedG; + return expression.semanticEquals(resolvedG); + }))) { return; } 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 bde3494dc7e77..c533c18188572 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 @@ -508,6 +508,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() { From 3b755be297c07d46e7a2e33b680e055758de7eb4 Mon Sep 17 00:00:00 2001 From: Andras Palinkas Date: Wed, 3 Mar 2021 19:07:52 -0500 Subject: [PATCH 3/5] PR suggestions Recursive get/getDefault() to resolve() in AttributeMap Removed noise Test breakdown --- .../xpack/ql/expression/AttributeMap.java | 43 ++++++++----------- .../ql/expression/AttributeMapTests.java | 21 ++++----- .../src/main/resources/subselect.sql-spec | 6 +++ .../xpack/sql/analysis/analyzer/Analyzer.java | 4 +- .../xpack/sql/analysis/analyzer/Verifier.java | 20 ++++----- .../xpack/sql/optimizer/Optimizer.java | 10 ++--- .../xpack/sql/planner/QueryFolder.java | 9 ++-- .../querydsl/container/QueryContainer.java | 14 +++--- .../sql/planner/QueryTranslatorTests.java | 27 ++++++++++-- 9 files changed, 85 insertions(+), 69 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 985ed6027c8fa..92b06828a1d70 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 @@ -148,6 +148,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; @@ -248,42 +250,31 @@ public boolean containsValue(Object value) { return delegate.containsValue(value); } - /** - * @param key {@link NamedExpression} to look up - * @return Looks up the key in the AttributeMap recursively, meaning if the value for this key - * is another key in the map, it will follow it. It will return the final value or null if the key - * does not exist in the map. - */ @Override public E get(Object key) { - return getOrDefault(key, null); + if (key instanceof NamedExpression) { + return delegate.get(new AttributeWrapper(((NamedExpression) key).toAttribute())); + } + return null; } - /** - * @param key {@link NamedExpression} to look up - * @return Looks up the key in the AttributeMap recursively, meaning if the value for this key - * is another key in the map, it will follow it. It will return the final value or the - * specified default value if the key does not exist in the map. - */ @Override public E getOrDefault(Object key, E defaultValue) { - E candidate = defaultValue; - E value = null; - while (((value = lookup(key)) != null || containsKey(key)) && value != key) { - key = candidate = value; + if (key instanceof NamedExpression) { + return delegate.getOrDefault(new AttributeWrapper(((NamedExpression) key).toAttribute()), defaultValue); } - return candidate; + return defaultValue; } - /** - * @param key {@link NamedExpression} to look up - * @return Result of the non-recursive lookup of the key in the map. - */ - private E lookup(Object key) { - if (key instanceof NamedExpression) { - return delegate.get(new AttributeWrapper(((NamedExpression) key).toAttribute())); + @SuppressWarnings({ "unchecked" }) + public E resolve(Object key, E defaultValue) { + AttributeMap map = (AttributeMap) this; + Object candidate = defaultValue; + Object value = null; + while ((value = map.getOrDefault(key, NOT_FOUND)) != NOT_FOUND && value != key) { + key = candidate = value; } - return null; + return (E) candidate; } @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 85352b906a0ff..f4b9f5e6c3223 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 @@ -63,7 +63,7 @@ public void testAttributeMapWithSameAliasesCanResolveAttributes() { assertTrue(newAttributeMap.get(param2.toAttribute()) == param2.child()); } - public void testResolveRecursively() { + public void testResolve() { AttributeMap.Builder builder = AttributeMap.builder(); Attribute one = a("one"); Attribute two = fieldAttribute("two", DataTypes.INTEGER); @@ -77,19 +77,14 @@ public void testResolveRecursively() { builder.put(threeAliasAlias.toAttribute(), threeAliasAlias.child()); AttributeMap map = builder.build(); - assertEquals(of("one"), map.get(one)); - assertEquals(map.get(one), map.getOrDefault(one, null)); - assertEquals("two", map.get(two)); - assertEquals(map.get(two), map.getOrDefault(two, null)); - assertEquals(of("three"), map.get(three)); - assertEquals(map.get(three), map.getOrDefault(three, null)); - assertEquals(map.get(three), map.getOrDefault(threeAlias, null)); - assertEquals(map.get(three), map.get(threeAlias)); - assertEquals(map.get(three), map.getOrDefault(threeAliasAlias, null)); - assertEquals(map.get(three), map.get(threeAliasAlias)); + assertEquals(of("one"), map.resolve(one, null)); + assertEquals("two", map.resolve(two, null)); + assertEquals(of("three"), map.resolve(three, null)); + assertEquals(of("three"), map.resolve(threeAlias, null)); + assertEquals(of("three"), map.resolve(threeAliasAlias, null)); Attribute four = a("four"); - assertEquals("not found", map.getOrDefault(four, "not found")); - assertNull(map.get(four)); + assertEquals("not found", map.resolve(four, "not found")); + assertNull(map.resolve(four, null)); } private Alias createIntParameterAlias(int index, int value) { 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 2b346ad6fc6cd..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 @@ -35,6 +35,12 @@ FROM ( ) 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; 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 b86f519ab57ff..a0c946d8ab99a 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 @@ -317,7 +317,7 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur o.order().forEach(oe -> { final Expression e = oe.child(); - final Expression resolvedE = attributeRefs.getOrDefault(e, e); + final Expression resolvedE = attributeRefs.resolve(e, e); // aggregates are allowed if (Functions.isAggregate(resolvedE)) { @@ -343,7 +343,7 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur // Also, make sure to compare attributes directly if (resolvedE.anyMatch(expression -> Expressions.anyMatch(groupingAndMatchingAggregatesAliases, g -> { - Expression resolvedG = attributeRefs.getOrDefault(g, g); + Expression resolvedG = attributeRefs.resolve(g, g); resolvedG = expression instanceof Attribute ? Expressions.attribute(resolvedG) : resolvedG; return expression.semanticEquals(resolvedG); }))) { @@ -411,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())); @@ -584,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", @@ -695,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))); } @@ -722,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)); @@ -744,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 && @@ -763,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 9678bcb38e9ae..915f747188c0a 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 @@ -131,8 +131,8 @@ protected Iterable.Batch> batches() { ); Batch refs = new Batch("Replace References", Limiter.ONCE, - new ReplaceReferenceAttributeWithSource() - ); + new ReplaceReferenceAttributeWithSource() + ); Batch operators = new Batch("Operator Optimization", // combining @@ -223,7 +223,7 @@ public LogicalPlan apply(LogicalPlan plan) { // collect aliases plan.forEachExpressionUp(Alias.class, a -> builder.put(a.toAttribute(), a.child())); final AttributeMap collectRefs = builder.build(); - java.util.function.Function replaceReference = r -> collectRefs.getOrDefault(r, r); + 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, null); 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 02f9a02e061a8..5e117531ce0a3 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,12 +600,13 @@ else if (target.foldable()) { else { GroupByKey matchingGroup = null; if (groupingContext != null) { - final Expression resolvedTarget = queryC.aliases().getOrDefault(target, target); - matchingGroup = groupingContext.groupFor(resolvedTarget); + 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())), Expressions.id(resolvedTarget)); + new GroupByRef(matchingGroup.id(), null, isDateBased(ne.dataType())), id); } // fallback else { @@ -709,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, null); } 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..4ab0bd9b3604d 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)) == 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/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index af1593e22c637..a149d07fa3abd 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 @@ -2658,24 +2658,43 @@ public void testSubqueryWithAliasOrderByAlias() throws Exception { "ORDER BY s.i > 10"); } - public void testReferenceResolutionInSubqueries() { + 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" + + " 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 " + + " 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"); - optimizeAndPlan("SELECT g FROM (SELECT date AS f, int AS g FROM test) WHERE g IS NOT NULL GROUP BY g ORDER BY g ASC"); + } + + 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") From 932c01cc6b7d3a072412a14782b35434e941aa1c Mon Sep 17 00:00:00 2001 From: Andras Palinkas Date: Wed, 10 Mar 2021 18:07:22 -0500 Subject: [PATCH 4/5] Bugfix and PR suggestions --- .../xpack/ql/expression/AttributeMap.java | 26 +++++++--- .../ql/expression/AttributeMapTests.java | 47 ++++++++++++++++--- .../xpack/sql/analysis/analyzer/Verifier.java | 4 +- .../xpack/sql/optimizer/Optimizer.java | 2 +- .../xpack/sql/planner/QueryFolder.java | 2 +- .../querydsl/container/QueryContainer.java | 2 +- 6 files changed, 65 insertions(+), 18 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 92b06828a1d70..6b47cce425e3a 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; @@ -266,15 +268,25 @@ public E getOrDefault(Object key, E defaultValue) { return defaultValue; } - @SuppressWarnings({ "unchecked" }) + public E resolve(Object key) { + return resolve(key, null); + } + public E resolve(Object key, E defaultValue) { - AttributeMap map = (AttributeMap) this; - Object candidate = defaultValue; - Object value = null; - while ((value = map.getOrDefault(key, NOT_FOUND)) != NOT_FOUND && value != key) { - key = candidate = value; + 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 = value = candidate; } - return (E) 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 f4b9f5e6c3223..9045b96324d97 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; @@ -77,14 +78,48 @@ public void testResolve() { builder.put(threeAliasAlias.toAttribute(), threeAliasAlias.child()); AttributeMap map = builder.build(); - assertEquals(of("one"), map.resolve(one, null)); - assertEquals("two", map.resolve(two, null)); - assertEquals(of("three"), map.resolve(three, null)); - assertEquals(of("three"), map.resolve(threeAlias, null)); - assertEquals(of("three"), map.resolve(threeAliasAlias, null)); + 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, null)); + 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) { 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 a0c946d8ab99a..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 @@ -411,7 +411,7 @@ private static boolean checkGroupByHavingHasOnlyAggs(Expression e, Set source, List { private void findNested(Expression exp, AttributeMap functions, Consumer onFind) { exp.forEachUp(e -> { if (e instanceof ReferenceAttribute) { - Function f = functions.resolve(e, null); + Function f = functions.resolve(e); if (f != null) { findNested(f, functions, onFind); } 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 5e117531ce0a3..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 @@ -710,7 +710,7 @@ protected PhysicalPlan rule(OrderExec plan) { // if it's a reference, get the target expression if (orderExpression instanceof ReferenceAttribute) { - orderExpression = qContainer.aliases().resolve(orderExpression, null); + 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 4ab0bd9b3604d..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 @@ -376,7 +376,7 @@ 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 = null; - if ((proc = scalarFunctions.resolve(attr, null)) == null) { + if ((proc = scalarFunctions.resolve(attr)) == null) { proc = function.asPipe(); scalarFunctions = AttributeMap.builder(scalarFunctions).put(attr, proc).build(); } From 1cb642d4b058d7d61b9c90817fbaddc219565da1 Mon Sep 17 00:00:00 2001 From: Andras Palinkas Date: Thu, 11 Mar 2021 09:51:22 -0500 Subject: [PATCH 5/5] PR suggestions --- .../org/elasticsearch/xpack/ql/expression/AttributeMap.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 6b47cce425e3a..5b6e1654d9c59 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 @@ -284,7 +284,8 @@ public E resolve(Object key, E defaultValue) { if (--allowedLookups == 0) { throw new QlIllegalArgumentException("Potential cycle detected"); } - key = value = candidate; + key = candidate; + value = candidate; } return value; }