From 545be942a3f4c30ea4af96b1d070e7d20865283c Mon Sep 17 00:00:00 2001 From: Luegg Date: Mon, 2 Aug 2021 14:01:00 +0200 Subject: [PATCH 1/5] Disallow queries with inner limits that cannot be executed correctly --- .../server/src/main/resources/select.sql-spec | 4 -- .../xpack/sql/parser/LogicalPlanBuilder.java | 20 ++++++ .../xpack/sql/planner/Verifier.java | 33 +++++---- .../xpack/sql/parser/SqlParserTests.java | 5 ++ .../xpack/sql/planner/VerifierTests.java | 70 ++++++++++++++++--- 5 files changed, 105 insertions(+), 27 deletions(-) diff --git a/x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec b/x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec index 304945c0dacc0..0a6d0558c20ab 100644 --- a/x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec +++ b/x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec @@ -153,14 +153,10 @@ selectOrderByOrderByOrderByLimit SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no DESC) ORDER BY emp_no ASC) ORDER BY emp_no DESC LIMIT 5; selectOrderByOrderByOrderByLimitLimit SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no DESC) ORDER BY emp_no ASC) ORDER BY emp_no DESC LIMIT 12) LIMIT 6; -selectOrderByLimitSameOrderBy -SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp LIMIT 10) ORDER BY emp_no) ORDER BY emp_no LIMIT 5; selectGroupByOrderByLimit SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages) ORDER BY max DESC LIMIT 3; selectGroupByOrderByLimitNulls SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages) ORDER BY max DESC NULLS FIRST LIMIT 3; -selectGroupByLimitOrderByLimit -SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp WHERE languages IS NOT NULL GROUP BY languages LIMIT 5) ORDER BY max DESC LIMIT 3; selectGroupByOrderByOrderByLimit SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages ORDER BY max ASC) ORDER BY max DESC NULLS FIRST LIMIT 4; selectGroupByOrderByOrderByLimitNulls diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java index 1d38709b03c4a..f7a4ed9329d9f 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java @@ -23,6 +23,7 @@ import org.elasticsearch.xpack.ql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.ql.plan.logical.OrderBy; import org.elasticsearch.xpack.ql.plan.logical.Project; +import org.elasticsearch.xpack.ql.plan.logical.UnaryPlan; import org.elasticsearch.xpack.ql.plan.logical.UnresolvedRelation; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataTypes; @@ -58,6 +59,7 @@ import java.time.ZoneId; import java.util.ArrayList; +import java.util.Arrays; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -101,6 +103,9 @@ public LogicalPlan visitQueryNoWith(QueryNoWithContext ctx) { List orders = ctx.orderBy(); OrderByContext endContext = orders.get(orders.size() - 1); plan = new OrderBy(source(ctx.ORDER(), endContext), plan, visitList(ctx.orderBy(), Order.class)); + + // Limit from TOP clauses must be the parent of the OrderBy clause + plan = pullUpTopClauseFromQuerySpecification(plan); } LimitClauseContext limitClause = ctx.limitClause(); @@ -119,6 +124,21 @@ public LogicalPlan visitQueryNoWith(QueryNoWithContext ctx) { return plan; } + private LogicalPlan pullUpTopClauseFromQuerySpecification(LogicalPlan plan) { + if (plan instanceof UnaryPlan) { + UnaryPlan unary = (UnaryPlan) plan; + if (unary.child() instanceof Limit) { + Limit limit = (Limit) unary.child(); + + return limit.replaceChildrenSameSize(Arrays.asList( + unary.replaceChildrenSameSize(Arrays.asList( + limit.child())))); + } + } + + return plan; + } + @Override public LogicalPlan visitQuerySpecification(QuerySpecificationContext ctx) { LogicalPlan query; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java index ed5b5d017c682..b9c3acd52f6f3 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java @@ -7,16 +7,20 @@ package org.elasticsearch.xpack.sql.planner; import org.elasticsearch.xpack.ql.common.Failure; -import org.elasticsearch.xpack.ql.expression.Order; -import org.elasticsearch.xpack.ql.util.Holder; +import org.elasticsearch.xpack.sql.plan.physical.AggregateExec; +import org.elasticsearch.xpack.sql.plan.physical.FilterExec; import org.elasticsearch.xpack.sql.plan.physical.LimitExec; import org.elasticsearch.xpack.sql.plan.physical.OrderExec; import org.elasticsearch.xpack.sql.plan.physical.PhysicalPlan; +import org.elasticsearch.xpack.sql.plan.physical.PivotExec; import org.elasticsearch.xpack.sql.plan.physical.Unexecutable; import org.elasticsearch.xpack.sql.plan.physical.UnplannedExec; import java.util.ArrayList; +import java.util.Comparator; import java.util.List; +import java.util.Set; +import java.util.TreeSet; import static org.elasticsearch.xpack.ql.common.Failure.fail; @@ -59,20 +63,19 @@ static List verifyExecutingPlan(PhysicalPlan plan) { } private static void checkForNonCollapsableSubselects(PhysicalPlan plan, List failures) { - Holder hasLimit = new Holder<>(Boolean.FALSE); - Holder> orderBy = new Holder<>(); - plan.forEachUp(p -> { - if (hasLimit.get() == false && p instanceof LimitExec) { - hasLimit.set(Boolean.TRUE); - return; - } - if (p instanceof OrderExec) { - if (hasLimit.get() && orderBy.get() != null && ((OrderExec) p).order().equals(orderBy.get()) == false) { - failures.add(fail(p, "Cannot use ORDER BY on top of a subquery with ORDER BY and LIMIT")); - } else { - orderBy.set(((OrderExec) p).order()); - } + // use a comparator to ensure deterministic order of the error messages + Set limits = new TreeSet<>(Comparator.comparing(l -> l.source().text())); + + plan.forEachDown(p -> { + if (p instanceof OrderExec || p instanceof FilterExec || p instanceof PivotExec || p instanceof AggregateExec) { + p.forEachDown(LimitExec.class, limits::add); } }); + + for (LimitExec limit : limits) { + failures.add( + fail(limit, "LIMIT or TOP cannot be used in a subquery if outer query contains GROUP BY, WHERE, ORDER BY or PIVOT") + ); + } } } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/SqlParserTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/SqlParserTests.java index a95a84208e0c6..9719851fd1ec6 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/SqlParserTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/SqlParserTests.java @@ -142,9 +142,14 @@ public void testTop() { public void testUseBothTopAndLimitInvalid() { ParsingException e = expectThrows(ParsingException.class, () -> parseStatement("SELECT TOP 10 * FROM test LIMIT 20")); assertEquals("line 1:28: TOP and LIMIT are not allowed in the same query - use one or the other", e.getMessage()); + e = expectThrows(ParsingException.class, () -> parseStatement("SELECT TOP 30 a, count(*) cnt FROM test WHERE b = 20 GROUP BY a HAVING cnt > 10 LIMIT 40")); assertEquals("line 1:82: TOP and LIMIT are not allowed in the same query - use one or the other", e.getMessage()); + + e = expectThrows(ParsingException.class, + () -> parseStatement("SELECT TOP 30 * FROM test ORDER BY a LIMIT 40")); + assertEquals("line 1:39: TOP and LIMIT are not allowed in the same query - use one or the other", e.getMessage()); } public void testsSelectNonReservedKeywords() { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierTests.java index 728947adc13bb..32b4fbf19f27d 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierTests.java @@ -14,6 +14,8 @@ import org.elasticsearch.xpack.sql.analysis.analyzer.Verifier; import org.elasticsearch.xpack.sql.expression.function.SqlFunctionRegistry; import org.elasticsearch.xpack.sql.parser.SqlParser; +import org.elasticsearch.xpack.sql.plan.physical.EsQueryExec; +import org.elasticsearch.xpack.sql.plan.physical.PhysicalPlan; import org.elasticsearch.xpack.sql.stats.Metrics; import static org.elasticsearch.xpack.sql.SqlTestUtils.TEST_CFG; @@ -33,10 +35,14 @@ public class VerifierTests extends ESTestCase { ); private final Planner planner = new Planner(); + private PhysicalPlan verify(String sql) { + return planner.plan(analyzer.analyze(parser.createStatement(sql), true), true); + } + private String error(String sql) { PlanningException e = expectThrows( PlanningException.class, - () -> planner.plan(analyzer.analyze(parser.createStatement(sql), true), true) + () -> verify(sql) ); String message = e.getMessage(); assertTrue(message.startsWith("Found ")); @@ -45,21 +51,28 @@ private String error(String sql) { return message.substring(index + pattern.length()); } + private String innerLimitMsg(int line, int column) { + return line + + ":" + + column + + ": LIMIT or TOP cannot be used in a subquery if outer query contains GROUP BY, WHERE, ORDER BY or PIVOT"; + } + public void testSubselectWithOrderByOnTopOfOrderByAndLimit() { assertEquals( - "1:60: Cannot use ORDER BY on top of a subquery with ORDER BY and LIMIT", + innerLimitMsg(1, 50), error("SELECT * FROM (SELECT * FROM test ORDER BY 1 ASC LIMIT 10) ORDER BY 2") ); assertEquals( - "1:72: Cannot use ORDER BY on top of a subquery with ORDER BY and LIMIT", + innerLimitMsg(1, 50), error("SELECT * FROM (SELECT * FROM (SELECT * FROM test LIMIT 10) ORDER BY 1) ORDER BY 2") ); assertEquals( - "1:75: Cannot use ORDER BY on top of a subquery with ORDER BY and LIMIT", + innerLimitMsg(1, 66), error("SELECT * FROM (SELECT * FROM (SELECT * FROM test ORDER BY 1 ASC) LIMIT 5) ORDER BY 1 DESC") ); assertEquals( - "1:152: Cannot use ORDER BY on top of a subquery with ORDER BY and LIMIT", + innerLimitMsg(1, 142), error("SELECT * FROM (" + "SELECT * FROM (" + "SELECT * FROM (" + @@ -68,17 +81,21 @@ public void testSubselectWithOrderByOnTopOfOrderByAndLimit() { "ORDER BY int DESC NULLS LAST LIMIT 12) " + "ORDER BY int DESC NULLS FIRST") ); + assertEquals( + innerLimitMsg(1, 50) + "\nline " + innerLimitMsg(1, 71), + error("SELECT * FROM (SELECT * FROM (SELECT * FROM test LIMIT 10) ORDER BY 1 LIMIT 20) ORDER BY 2") + ); } public void testSubselectWithOrderByOnTopOfGroupByOrderByAndLimit() { assertEquals( - "1:96: Cannot use ORDER BY on top of a subquery with ORDER BY and LIMIT", + innerLimitMsg(1, 86), error( "SELECT * FROM (SELECT max(int) AS max, bool FROM test GROUP BY bool ORDER BY max ASC LIMIT 10) ORDER BY max DESC" ) ); assertEquals( - "1:112: Cannot use ORDER BY on top of a subquery with ORDER BY and LIMIT", + innerLimitMsg(1, 102), error( "SELECT * FROM (" + "SELECT * FROM (" @@ -88,7 +105,7 @@ public void testSubselectWithOrderByOnTopOfGroupByOrderByAndLimit() { ) ); assertEquals( - "1:186: Cannot use ORDER BY on top of a subquery with ORDER BY and LIMIT", + innerLimitMsg(1, 176), error("SELECT * FROM (" + "SELECT * FROM (" + "SELECT * FROM (" + @@ -98,4 +115,41 @@ public void testSubselectWithOrderByOnTopOfGroupByOrderByAndLimit() { "ORDER BY max DESC NULLS FIRST") ); } + + public void testInnerLimitWithWhere() { + assertEquals(innerLimitMsg(1, 35), + error("SELECT * FROM (SELECT * FROM test LIMIT 10) WHERE int = 1")); + assertEquals(innerLimitMsg(1, 50), + error("SELECT * FROM (SELECT * FROM (SELECT * FROM test LIMIT 10)) WHERE int = 1")); + assertEquals(innerLimitMsg(1, 51), + error("SELECT * FROM (SELECT * FROM (SELECT * FROM test) LIMIT 10) WHERE int = 1")); + } + + public void testInnerLimitWithGroupBy() { + assertEquals(innerLimitMsg(1, 37), + error("SELECT int FROM (SELECT * FROM test LIMIT 10) GROUP BY int")); + assertEquals(innerLimitMsg(1, 52), + error("SELECT int FROM (SELECT * FROM (SELECT * FROM test LIMIT 10)) GROUP BY int")); + assertEquals(innerLimitMsg(1, 53), + error("SELECT int FROM (SELECT * FROM (SELECT * FROM test) LIMIT 10) GROUP BY int")); + } + + public void testInnerLimitWithPivot() { + assertEquals(innerLimitMsg(1, 52), + error("SELECT * FROM (SELECT int, bool, keyword FROM test LIMIT 10) PIVOT (AVG(int) FOR bool IN (true, false))")); + } + + public void testTopWithOrderBySucceeds() { + PhysicalPlan plan = verify("SELECT TOP 5 * FROM test ORDER BY int"); + assertEquals(EsQueryExec.class, plan.getClass()); + } + + public void testInnerTop() { + assertEquals(innerLimitMsg(1, 23), + error("SELECT * FROM (SELECT TOP 10 * FROM test) WHERE int = 1")); + assertEquals(innerLimitMsg(1, 23), + error("SELECT * FROM (SELECT TOP 10 * FROM test) ORDER BY int")); + assertEquals(innerLimitMsg(1, 23), + error("SELECT * FROM (SELECT TOP 10 int, bool, keyword FROM test) PIVOT (AVG(int) FOR bool IN (true, false))")); + } } From 264e042c893d99ee0ecaa2e03e0607cbdfb0ee65 Mon Sep 17 00:00:00 2001 From: Luegg Date: Tue, 3 Aug 2021 16:05:34 +0200 Subject: [PATCH 2/5] address review comments --- .../xpack/sql/parser/LogicalPlanBuilder.java | 31 ++++++------------- .../xpack/sql/planner/Verifier.java | 23 ++++++++------ .../xpack/sql/planner/VerifierTests.java | 4 +-- 3 files changed, 25 insertions(+), 33 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java index f7a4ed9329d9f..117672c07dc69 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java @@ -23,7 +23,6 @@ import org.elasticsearch.xpack.ql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.ql.plan.logical.OrderBy; import org.elasticsearch.xpack.ql.plan.logical.Project; -import org.elasticsearch.xpack.ql.plan.logical.UnaryPlan; import org.elasticsearch.xpack.ql.plan.logical.UnresolvedRelation; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataTypes; @@ -59,7 +58,7 @@ import java.time.ZoneId; import java.util.ArrayList; -import java.util.Arrays; +import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -102,10 +101,15 @@ public LogicalPlan visitQueryNoWith(QueryNoWithContext ctx) { if (ctx.orderBy().isEmpty() == false) { List orders = ctx.orderBy(); OrderByContext endContext = orders.get(orders.size() - 1); - plan = new OrderBy(source(ctx.ORDER(), endContext), plan, visitList(ctx.orderBy(), Order.class)); - - // Limit from TOP clauses must be the parent of the OrderBy clause - plan = pullUpTopClauseFromQuerySpecification(plan); + Source source = source(ctx.ORDER(), endContext); + List order = visitList(ctx.orderBy(), Order.class); + + if (plan instanceof Limit) { + // Limit from TOP clauses must be the parent of the OrderBy clause + plan = plan.replaceChildrenSameSize(Collections.singletonList(new OrderBy(source, ((Limit) plan).child(), order))); + } else { + plan = new OrderBy(source, plan, order); + } } LimitClauseContext limitClause = ctx.limitClause(); @@ -124,21 +128,6 @@ public LogicalPlan visitQueryNoWith(QueryNoWithContext ctx) { return plan; } - private LogicalPlan pullUpTopClauseFromQuerySpecification(LogicalPlan plan) { - if (plan instanceof UnaryPlan) { - UnaryPlan unary = (UnaryPlan) plan; - if (unary.child() instanceof Limit) { - Limit limit = (Limit) unary.child(); - - return limit.replaceChildrenSameSize(Arrays.asList( - unary.replaceChildrenSameSize(Arrays.asList( - limit.child())))); - } - } - - return plan; - } - @Override public LogicalPlan visitQuerySpecification(QuerySpecificationContext ctx) { LogicalPlan query; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java index b9c3acd52f6f3..d541564a1ed23 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java @@ -7,20 +7,19 @@ package org.elasticsearch.xpack.sql.planner; import org.elasticsearch.xpack.ql.common.Failure; +import org.elasticsearch.xpack.ql.util.Holder; import org.elasticsearch.xpack.sql.plan.physical.AggregateExec; import org.elasticsearch.xpack.sql.plan.physical.FilterExec; import org.elasticsearch.xpack.sql.plan.physical.LimitExec; import org.elasticsearch.xpack.sql.plan.physical.OrderExec; import org.elasticsearch.xpack.sql.plan.physical.PhysicalPlan; import org.elasticsearch.xpack.sql.plan.physical.PivotExec; +import org.elasticsearch.xpack.sql.plan.physical.UnaryExec; import org.elasticsearch.xpack.sql.plan.physical.Unexecutable; import org.elasticsearch.xpack.sql.plan.physical.UnplannedExec; import java.util.ArrayList; -import java.util.Comparator; import java.util.List; -import java.util.Set; -import java.util.TreeSet; import static org.elasticsearch.xpack.ql.common.Failure.fail; @@ -63,18 +62,22 @@ static List verifyExecutingPlan(PhysicalPlan plan) { } private static void checkForNonCollapsableSubselects(PhysicalPlan plan, List failures) { - // use a comparator to ensure deterministic order of the error messages - Set limits = new TreeSet<>(Comparator.comparing(l -> l.source().text())); + Holder limit = new Holder<>(); + Holder limitedExec = new Holder<>(); - plan.forEachDown(p -> { - if (p instanceof OrderExec || p instanceof FilterExec || p instanceof PivotExec || p instanceof AggregateExec) { - p.forEachDown(LimitExec.class, limits::add); + plan.forEachUp(p -> { + if (limit.get() == null && p instanceof LimitExec) { + limit.set((LimitExec) p); + } else if (limit.get() != null && limitedExec.get() == null) { + if (p instanceof OrderExec || p instanceof FilterExec || p instanceof PivotExec || p instanceof AggregateExec) { + limitedExec.set((UnaryExec) p); + } } }); - for (LimitExec limit : limits) { + if (limitedExec.get() != null) { failures.add( - fail(limit, "LIMIT or TOP cannot be used in a subquery if outer query contains GROUP BY, WHERE, ORDER BY or PIVOT") + fail(limit.get(), "LIMIT or TOP cannot be used in a subquery if outer query contains GROUP BY, ORDER BY, PIVOT or WHERE") ); } } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierTests.java index 32b4fbf19f27d..8c04f57fb4fac 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierTests.java @@ -55,7 +55,7 @@ private String innerLimitMsg(int line, int column) { return line + ":" + column - + ": LIMIT or TOP cannot be used in a subquery if outer query contains GROUP BY, WHERE, ORDER BY or PIVOT"; + + ": LIMIT or TOP cannot be used in a subquery if outer query contains GROUP BY, ORDER BY, PIVOT or WHERE"; } public void testSubselectWithOrderByOnTopOfOrderByAndLimit() { @@ -82,7 +82,7 @@ public void testSubselectWithOrderByOnTopOfOrderByAndLimit() { "ORDER BY int DESC NULLS FIRST") ); assertEquals( - innerLimitMsg(1, 50) + "\nline " + innerLimitMsg(1, 71), + innerLimitMsg(1, 50), error("SELECT * FROM (SELECT * FROM (SELECT * FROM test LIMIT 10) ORDER BY 1 LIMIT 20) ORDER BY 2") ); } From 8d8d992c188948e040eb57e3fa743ee8ba804a58 Mon Sep 17 00:00:00 2001 From: Luegg Date: Wed, 4 Aug 2021 10:23:35 +0200 Subject: [PATCH 3/5] introduce PushDownAndCombineOrderBy optimization --- .../server/src/main/resources/select.sql-spec | 20 +++++++ .../xpack/sql/optimizer/Optimizer.java | 59 ++++++++++++++++++- .../xpack/sql/planner/QueryFolder.java | 4 ++ .../xpack/sql/planner/QueryFolderTests.java | 29 +++++++++ 4 files changed, 111 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec b/x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec index 0a6d0558c20ab..70c3c43e8b2a8 100644 --- a/x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec +++ b/x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec @@ -153,6 +153,8 @@ selectOrderByOrderByOrderByLimit SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no DESC) ORDER BY emp_no ASC) ORDER BY emp_no DESC LIMIT 5; selectOrderByOrderByOrderByLimitLimit SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no DESC) ORDER BY emp_no ASC) ORDER BY emp_no DESC LIMIT 12) LIMIT 6; +selectOrderByLimitSameOrderBy +SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no LIMIT 10)) ORDER BY emp_no LIMIT 5; selectGroupByOrderByLimit SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages) ORDER BY max DESC LIMIT 3; selectGroupByOrderByLimitNulls @@ -165,3 +167,21 @@ selectGroupByWithAliasedSubQuery SELECT max, languages FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages ORDER BY max ASC NULLS LAST) AS subquery; selectConstantFromSubQuery SELECT * FROM (SELECT * FROM (SELECT 1)); + +// +// SELECT with multiple ORDER BY +// +selectTwoDistinctOrderBy +SELECT * FROM (SELECT * FROM test_emp ORDER BY gender NULLS FIRST) ORDER BY languages NULLS FIRST; +selectThreeDistinctOrderBy +SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY salary) ORDER BY languages NULLS FIRST) ORDER BY gender NULLS FIRST; +selectTwoDistinctOrderByWithDuplicate1 +SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY salary) ORDER BY languages NULLS FIRST) ORDER BY salary; +selectTwoDistinctOrderByWithDuplicate2 +SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY salary) ORDER BY salary) ORDER BY languages NULLS FIRST; +selectTwoOrderByWithDistinctOrder +SELECT * FROM (SELECT * FROM test_emp ORDER BY gender ASC NULLS FIRST) ORDER BY gender DESC NULLS LAST; +selectTwoOrderByWithShadowedOrder +SELECT * FROM (SELECT * FROM test_emp ORDER BY salary, gender ASC, languages NULLS LAST) ORDER BY languages NULLS FIRST, gender DESC NULLS LAST; +selectTwoOrderByWithWhere +SELECT * FROM (SELECT * FROM test_emp ORDER BY gender NULLS FIRST) WHERE salary > 10000 ORDER BY languages NULLS FIRST; 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 fdc4ffea3efb3..ec33f7d14aa7d 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 @@ -22,6 +22,7 @@ import org.elasticsearch.xpack.ql.expression.ReferenceAttribute; import org.elasticsearch.xpack.ql.expression.UnresolvedAttribute; import org.elasticsearch.xpack.ql.expression.function.Function; +import org.elasticsearch.xpack.ql.expression.function.Functions; import org.elasticsearch.xpack.ql.expression.function.aggregate.AggregateFunction; import org.elasticsearch.xpack.ql.expression.function.aggregate.Count; import org.elasticsearch.xpack.ql.expression.function.aggregate.InnerAggregate; @@ -93,6 +94,7 @@ import java.time.ZoneId; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.LinkedHashSet; @@ -165,6 +167,9 @@ protected Iterable.Batch> batches() { new PushDownAndCombineFilters() ); + Batch pushDownOrderBy = new Batch("Push Down Order By", + new PushDownAndCombineOrderBy()); + Batch aggregate = new Batch("Aggregation Rewrite", new ReplaceMinMaxWithTopHits(), new ReplaceAggsWithMatrixStats(), @@ -189,7 +194,7 @@ protected Iterable.Batch> batches() { CleanAliases.INSTANCE, new SetAsOptimized()); - return Arrays.asList(substitutions, refs, operators, aggregate, local, label); + return Arrays.asList(substitutions, refs, operators, pushDownOrderBy, aggregate, local, label); } static class RewritePivot extends OptimizerRule { @@ -1255,6 +1260,58 @@ private boolean foldable(Expression e) { } + static class PushDownAndCombineOrderBy extends OptimizerRule { + + @Override + protected LogicalPlan rule(OrderBy plan) { + if (plan.child() instanceof OrderBy) { + OrderBy child = (OrderBy) plan.child(); + List combinedOrder = combineOrders(plan.order(), child.order()); + + return new OrderBy(child.source(), child.child(), combinedOrder); + + } else if (plan.child() instanceof Filter || plan.child() instanceof Project) { + UnaryPlan child = (UnaryPlan) plan.child(); + Holder hasAggregate = new Holder<>(false); + if (child instanceof Filter) { + child.forEachExpressionDown(e -> hasAggregate.set(hasAggregate.get() || Functions.isAggregate(e))); + } + + if (hasAggregate.get() == false) { + return child.replaceChildrenSameSize( + Collections.singletonList(plan.replaceChildrenSameSize(Collections.singletonList(child.child()))) + ); + } + + } else if (plan.child() instanceof Limit) { + // OrderBy can be pushed through a Limit if the Limit's child is also an OrderBy and the two sort orders are equal + Limit child = (Limit) plan.child(); + if (child.child() instanceof OrderBy) { + OrderBy innerOrderBy = (OrderBy) child.child(); + List combinedOrder = combineOrders(plan.order(), innerOrderBy.order()); + + if (combinedOrder.equals(innerOrderBy.order())) { + return child.replaceChildrenSameSize(Collections.singletonList(innerOrderBy)); + } + } + } + + return plan; + } + + private List combineOrders(List parentOrders, List childOrders) { + List combined = parentOrders; + + for (Order childOrder : childOrders) { + if(combined.stream().noneMatch(parentOrder -> childOrder.child().equals(parentOrder.child()))){ + combined.add(childOrder); + } + } + + return combined; + } + } + abstract static class OptimizerBasicRule extends Rule { @Override 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 e0245804bb486..0d5b50b8fe62b 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 @@ -701,6 +701,10 @@ protected PhysicalPlan rule(OrderExec plan) { EsQueryExec exec = (EsQueryExec) plan.child(); QueryContainer qContainer = exec.queryContainer(); + if (qContainer.sort().isEmpty() == false) { + throw new SqlIllegalArgumentException("QueryContainer already defines sort order"); + } + for (Order order : plan.order()) { Direction direction = Direction.from(order.direction()); Missing missing = Missing.from(order.nullsPosition()); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java index f5a5d6c7591e7..ca64228e1fe73 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java @@ -12,6 +12,8 @@ import org.elasticsearch.xpack.ql.expression.function.aggregate.AggregateFunction; import org.elasticsearch.xpack.ql.index.EsIndex; import org.elasticsearch.xpack.ql.index.IndexResolution; +import org.elasticsearch.xpack.ql.querydsl.container.AttributeSort; +import org.elasticsearch.xpack.ql.querydsl.container.Sort; import org.elasticsearch.xpack.ql.type.EsField; import org.elasticsearch.xpack.sql.SqlTestUtils; import org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer; @@ -534,6 +536,33 @@ public void testPivotHasSameQueryAsGroupBy() { } } + public void testFoldRedundantOrderBy() { + PhysicalPlan p = plan("SELECT * FROM (SELECT * FROM test ORDER BY int LIMIT 10) ORDER BY int"); + assertEquals(EsQueryExec.class, p.getClass()); + EsQueryExec ee = (EsQueryExec) p; + assertEquals(10, ee.queryContainer().limit()); + assertEquals(1, ee.queryContainer().sort().size()); + AttributeSort as = (AttributeSort) ee.queryContainer().sort().values().toArray()[0]; + assertEquals("test.int", as.attribute().qualifiedName()); + } + + public void testFoldShadowedOrderBy() { + PhysicalPlan p = plan( + "SELECT * FROM (SELECT * FROM test ORDER BY int ASC, keyword NULLS LAST) ORDER BY keyword NULLS FIRST, int DESC" + ); + assertEquals(EsQueryExec.class, p.getClass()); + EsQueryExec ee = (EsQueryExec) p; + assertEquals(2, ee.queryContainer().sort().size()); + + AttributeSort as1 = (AttributeSort) ee.queryContainer().sort().values().toArray()[0]; + assertEquals("test.keyword", as1.attribute().qualifiedName()); + assertEquals(Sort.Missing.FIRST, as1.missing()); + + AttributeSort as2 = (AttributeSort) ee.queryContainer().sort().values().toArray()[1]; + assertEquals("test.int", as2.attribute().qualifiedName()); + assertEquals(Sort.Direction.DESC, as2.direction()); + } + private static String randomOrderByAndLimit(int noOfSelectArgs) { return SqlTestUtils.randomOrderByAndLimit(noOfSelectArgs, random()); } From a6ab2537f108c7dbc6ab555f5ec5318f14fd1c1d Mon Sep 17 00:00:00 2001 From: Luegg Date: Mon, 9 Aug 2021 08:19:14 +0200 Subject: [PATCH 4/5] Revert "introduce PushDownAndCombineOrderBy optimization" 8d8d992c188948e040eb57e3fa743ee8ba804a58 --- .../server/src/main/resources/select.sql-spec | 20 ------- .../xpack/sql/optimizer/Optimizer.java | 59 +------------------ .../xpack/sql/planner/QueryFolder.java | 4 -- .../xpack/sql/planner/QueryFolderTests.java | 29 --------- 4 files changed, 1 insertion(+), 111 deletions(-) diff --git a/x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec b/x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec index 70c3c43e8b2a8..0a6d0558c20ab 100644 --- a/x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec +++ b/x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec @@ -153,8 +153,6 @@ selectOrderByOrderByOrderByLimit SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no DESC) ORDER BY emp_no ASC) ORDER BY emp_no DESC LIMIT 5; selectOrderByOrderByOrderByLimitLimit SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no DESC) ORDER BY emp_no ASC) ORDER BY emp_no DESC LIMIT 12) LIMIT 6; -selectOrderByLimitSameOrderBy -SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no LIMIT 10)) ORDER BY emp_no LIMIT 5; selectGroupByOrderByLimit SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages) ORDER BY max DESC LIMIT 3; selectGroupByOrderByLimitNulls @@ -167,21 +165,3 @@ selectGroupByWithAliasedSubQuery SELECT max, languages FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages ORDER BY max ASC NULLS LAST) AS subquery; selectConstantFromSubQuery SELECT * FROM (SELECT * FROM (SELECT 1)); - -// -// SELECT with multiple ORDER BY -// -selectTwoDistinctOrderBy -SELECT * FROM (SELECT * FROM test_emp ORDER BY gender NULLS FIRST) ORDER BY languages NULLS FIRST; -selectThreeDistinctOrderBy -SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY salary) ORDER BY languages NULLS FIRST) ORDER BY gender NULLS FIRST; -selectTwoDistinctOrderByWithDuplicate1 -SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY salary) ORDER BY languages NULLS FIRST) ORDER BY salary; -selectTwoDistinctOrderByWithDuplicate2 -SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY salary) ORDER BY salary) ORDER BY languages NULLS FIRST; -selectTwoOrderByWithDistinctOrder -SELECT * FROM (SELECT * FROM test_emp ORDER BY gender ASC NULLS FIRST) ORDER BY gender DESC NULLS LAST; -selectTwoOrderByWithShadowedOrder -SELECT * FROM (SELECT * FROM test_emp ORDER BY salary, gender ASC, languages NULLS LAST) ORDER BY languages NULLS FIRST, gender DESC NULLS LAST; -selectTwoOrderByWithWhere -SELECT * FROM (SELECT * FROM test_emp ORDER BY gender NULLS FIRST) WHERE salary > 10000 ORDER BY languages NULLS FIRST; 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 ec33f7d14aa7d..fdc4ffea3efb3 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 @@ -22,7 +22,6 @@ import org.elasticsearch.xpack.ql.expression.ReferenceAttribute; import org.elasticsearch.xpack.ql.expression.UnresolvedAttribute; import org.elasticsearch.xpack.ql.expression.function.Function; -import org.elasticsearch.xpack.ql.expression.function.Functions; import org.elasticsearch.xpack.ql.expression.function.aggregate.AggregateFunction; import org.elasticsearch.xpack.ql.expression.function.aggregate.Count; import org.elasticsearch.xpack.ql.expression.function.aggregate.InnerAggregate; @@ -94,7 +93,6 @@ import java.time.ZoneId; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.LinkedHashSet; @@ -167,9 +165,6 @@ protected Iterable.Batch> batches() { new PushDownAndCombineFilters() ); - Batch pushDownOrderBy = new Batch("Push Down Order By", - new PushDownAndCombineOrderBy()); - Batch aggregate = new Batch("Aggregation Rewrite", new ReplaceMinMaxWithTopHits(), new ReplaceAggsWithMatrixStats(), @@ -194,7 +189,7 @@ protected Iterable.Batch> batches() { CleanAliases.INSTANCE, new SetAsOptimized()); - return Arrays.asList(substitutions, refs, operators, pushDownOrderBy, aggregate, local, label); + return Arrays.asList(substitutions, refs, operators, aggregate, local, label); } static class RewritePivot extends OptimizerRule { @@ -1260,58 +1255,6 @@ private boolean foldable(Expression e) { } - static class PushDownAndCombineOrderBy extends OptimizerRule { - - @Override - protected LogicalPlan rule(OrderBy plan) { - if (plan.child() instanceof OrderBy) { - OrderBy child = (OrderBy) plan.child(); - List combinedOrder = combineOrders(plan.order(), child.order()); - - return new OrderBy(child.source(), child.child(), combinedOrder); - - } else if (plan.child() instanceof Filter || plan.child() instanceof Project) { - UnaryPlan child = (UnaryPlan) plan.child(); - Holder hasAggregate = new Holder<>(false); - if (child instanceof Filter) { - child.forEachExpressionDown(e -> hasAggregate.set(hasAggregate.get() || Functions.isAggregate(e))); - } - - if (hasAggregate.get() == false) { - return child.replaceChildrenSameSize( - Collections.singletonList(plan.replaceChildrenSameSize(Collections.singletonList(child.child()))) - ); - } - - } else if (plan.child() instanceof Limit) { - // OrderBy can be pushed through a Limit if the Limit's child is also an OrderBy and the two sort orders are equal - Limit child = (Limit) plan.child(); - if (child.child() instanceof OrderBy) { - OrderBy innerOrderBy = (OrderBy) child.child(); - List combinedOrder = combineOrders(plan.order(), innerOrderBy.order()); - - if (combinedOrder.equals(innerOrderBy.order())) { - return child.replaceChildrenSameSize(Collections.singletonList(innerOrderBy)); - } - } - } - - return plan; - } - - private List combineOrders(List parentOrders, List childOrders) { - List combined = parentOrders; - - for (Order childOrder : childOrders) { - if(combined.stream().noneMatch(parentOrder -> childOrder.child().equals(parentOrder.child()))){ - combined.add(childOrder); - } - } - - return combined; - } - } - abstract static class OptimizerBasicRule extends Rule { @Override 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 0d5b50b8fe62b..e0245804bb486 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 @@ -701,10 +701,6 @@ protected PhysicalPlan rule(OrderExec plan) { EsQueryExec exec = (EsQueryExec) plan.child(); QueryContainer qContainer = exec.queryContainer(); - if (qContainer.sort().isEmpty() == false) { - throw new SqlIllegalArgumentException("QueryContainer already defines sort order"); - } - for (Order order : plan.order()) { Direction direction = Direction.from(order.direction()); Missing missing = Missing.from(order.nullsPosition()); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java index ca64228e1fe73..f5a5d6c7591e7 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java @@ -12,8 +12,6 @@ import org.elasticsearch.xpack.ql.expression.function.aggregate.AggregateFunction; import org.elasticsearch.xpack.ql.index.EsIndex; import org.elasticsearch.xpack.ql.index.IndexResolution; -import org.elasticsearch.xpack.ql.querydsl.container.AttributeSort; -import org.elasticsearch.xpack.ql.querydsl.container.Sort; import org.elasticsearch.xpack.ql.type.EsField; import org.elasticsearch.xpack.sql.SqlTestUtils; import org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer; @@ -536,33 +534,6 @@ public void testPivotHasSameQueryAsGroupBy() { } } - public void testFoldRedundantOrderBy() { - PhysicalPlan p = plan("SELECT * FROM (SELECT * FROM test ORDER BY int LIMIT 10) ORDER BY int"); - assertEquals(EsQueryExec.class, p.getClass()); - EsQueryExec ee = (EsQueryExec) p; - assertEquals(10, ee.queryContainer().limit()); - assertEquals(1, ee.queryContainer().sort().size()); - AttributeSort as = (AttributeSort) ee.queryContainer().sort().values().toArray()[0]; - assertEquals("test.int", as.attribute().qualifiedName()); - } - - public void testFoldShadowedOrderBy() { - PhysicalPlan p = plan( - "SELECT * FROM (SELECT * FROM test ORDER BY int ASC, keyword NULLS LAST) ORDER BY keyword NULLS FIRST, int DESC" - ); - assertEquals(EsQueryExec.class, p.getClass()); - EsQueryExec ee = (EsQueryExec) p; - assertEquals(2, ee.queryContainer().sort().size()); - - AttributeSort as1 = (AttributeSort) ee.queryContainer().sort().values().toArray()[0]; - assertEquals("test.keyword", as1.attribute().qualifiedName()); - assertEquals(Sort.Missing.FIRST, as1.missing()); - - AttributeSort as2 = (AttributeSort) ee.queryContainer().sort().values().toArray()[1]; - assertEquals("test.int", as2.attribute().qualifiedName()); - assertEquals(Sort.Direction.DESC, as2.direction()); - } - private static String randomOrderByAndLimit(int noOfSelectArgs) { return SqlTestUtils.randomOrderByAndLimit(noOfSelectArgs, random()); } From 2edf92d8d3323c793d1329d5ee065feb5f5944b6 Mon Sep 17 00:00:00 2001 From: Luegg Date: Mon, 9 Aug 2021 08:40:10 +0200 Subject: [PATCH 5/5] use replaceChild --- .../elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java index 117672c07dc69..5ed7085b9dbea 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java @@ -58,7 +58,6 @@ import java.time.ZoneId; import java.util.ArrayList; -import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -106,7 +105,8 @@ public LogicalPlan visitQueryNoWith(QueryNoWithContext ctx) { if (plan instanceof Limit) { // Limit from TOP clauses must be the parent of the OrderBy clause - plan = plan.replaceChildrenSameSize(Collections.singletonList(new OrderBy(source, ((Limit) plan).child(), order))); + Limit limit = (Limit) plan; + plan = limit.replaceChild(new OrderBy(source, limit.child(), order)); } else { plan = new OrderBy(source, plan, order); }