From f7a89ed5b1c14fdc037e19d8409e0dba7e4dff09 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 12 May 2021 17:46:59 +0300 Subject: [PATCH 1/8] SQL: Disallow non-collapsable subselects with ORDER BY Ordering an already pre-ordered and limited subselect is not allowed, as such queries cannot be collapsed and translated into query DSL, but the require an extra ordering step on top of the results returned internally by the search/agg query. Fixes: #71158 --- .../server/src/main/resources/select.sql-spec | 25 ++++++ .../xpack/sql/planner/Verifier.java | 22 +++++ .../xpack/sql/planner/VerifierErrorTests.java | 81 +++++++++++++++++++ 3 files changed, 128 insertions(+) create mode 100644 x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java 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 69fca33f026c0..01235d068b221 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 @@ -126,3 +126,28 @@ selectMathPIFromIndexWithWhereEvaluatingToFalse SELECT PI() AS pi FROM test_emp WHERE PI()=5; selectMathPIFromIndexWithWhereEvaluatingToFalseAndWithLimit SELECT PI() AS pi FROM test_emp WHERE PI()=5 LIMIT 3; + + +// +// SELECT with collapsable subqueries +// +selectOrderByLimit1 +SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no DESC) LIMIT 5; +selectOrderByLimit2 +SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no DESC)) LIMIT 10; +selectOrderByOrderByLimit1 +SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no DESC) ORDER BY emp_no ASC LIMIT 5; +selectOrderByOrderByLimit2 +SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no DESC) ORDER BY emp_no ASC) LIMIT 5; +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; +selectGroupByLimitOrderByLimit +SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages LIMIT 5) ORDER BY max DESC LIMIT 4; +selectGroupByOrderByOrderByLimit +SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages ORDER BY max ASC) ORDER BY max DESC LIMIT 4 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 3e52e317ca3fe..1b3de049548fa 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,6 +7,10 @@ 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.LimitExec; +import org.elasticsearch.xpack.sql.plan.physical.OrderExec; import org.elasticsearch.xpack.sql.plan.physical.PhysicalPlan; import org.elasticsearch.xpack.sql.plan.physical.Unexecutable; import org.elasticsearch.xpack.sql.plan.physical.UnplannedExec; @@ -32,6 +36,24 @@ static List verifyMappingPlan(PhysicalPlan plan) { }); }); + 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 (orderBy.get() == null && p instanceof OrderExec) { + orderBy.set(((OrderExec) p).order()); + return; + } + if (hasLimit.get() && orderBy.get() != null && p instanceof OrderExec) { + if (((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")); + } + } + }); + return failures; } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java new file mode 100644 index 0000000000000..d123c5dca3ccd --- /dev/null +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java @@ -0,0 +1,81 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.sql.planner; + +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.ql.index.EsIndex; +import org.elasticsearch.xpack.ql.index.IndexResolution; +import org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer; +import org.elasticsearch.xpack.sql.analysis.analyzer.VerificationException; +import org.elasticsearch.xpack.sql.expression.function.SqlFunctionRegistry; +import org.elasticsearch.xpack.sql.parser.SqlParser; +import org.elasticsearch.xpack.sql.stats.Metrics; + +import static org.elasticsearch.xpack.sql.SqlTestUtils.TEST_CFG; +import static org.elasticsearch.xpack.sql.types.SqlTypesTests.loadMapping; + +public class VerifierErrorTests extends ESTestCase { + + private final SqlParser parser = new SqlParser(); + private final IndexResolution indexResolution = IndexResolution.valid( + new EsIndex("test", loadMapping("mapping-multi-field-with-nested.json")) + ); + private final Analyzer analyzer = new Analyzer( + TEST_CFG, + new SqlFunctionRegistry(), + indexResolution, + new org.elasticsearch.xpack.sql.analysis.analyzer.Verifier(new Metrics()) + ); + private final Planner planner = new Planner(); + + private String error(String sql) { + PlanningException e = expectThrows( + PlanningException.class, + () -> planner.plan(analyzer.analyze(parser.createStatement(sql), true), true) + ); + String message = e.getMessage(); + assertTrue(message.startsWith("Found ")); + String pattern = "\nline "; + int index = message.indexOf(pattern); + return message.substring(index + pattern.length()); + } + + public void testSubselectWithOrderByOnTopOfOrderByAndLimit() { + assertEquals( + "1:60: Cannot use ORDER BY on top of a subquery with ORDER BY and LIMIT", + 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", + 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", + error("SELECT * FROM (SELECT * FROM (SELECT * FROM test ORDER BY 1 ASC) LIMIT 5) ORDER BY 1 DESC") + ); + } + + public void testSubselectWithOrderByOnTopOfGroupByOrderByAndLimit() { + assertEquals( + "1:96: Cannot use ORDER BY on top of a subquery with ORDER BY and LIMIT", + 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", + error( + "SELECT * FROM (" + + "SELECT * FROM (" + + "SELECT max(int) AS max, bool FROM test GROUP BY bool ORDER BY max ASC) " + + "LIMIT 10) " + + "ORDER BY max DESC" + ) + ); + } +} From d46d5436c87195b6bc54dbc66b19dde0deba80c4 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 12 May 2021 18:27:26 +0300 Subject: [PATCH 2/8] unused import --- .../org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java index d123c5dca3ccd..4050fd652eaa1 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java @@ -11,7 +11,6 @@ import org.elasticsearch.xpack.ql.index.EsIndex; import org.elasticsearch.xpack.ql.index.IndexResolution; import org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer; -import org.elasticsearch.xpack.sql.analysis.analyzer.VerificationException; import org.elasticsearch.xpack.sql.expression.function.SqlFunctionRegistry; import org.elasticsearch.xpack.sql.parser.SqlParser; import org.elasticsearch.xpack.sql.stats.Metrics; From 676cb9b81f8bba3605352e1ce64e6fdfcc326d75 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 12 May 2021 19:02:55 +0300 Subject: [PATCH 3/8] fix test --- x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec | 2 +- 1 file changed, 1 insertion(+), 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 01235d068b221..6d60ef61cfd9a 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 @@ -150,4 +150,4 @@ SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY langu selectGroupByLimitOrderByLimit SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages LIMIT 5) ORDER BY max DESC LIMIT 4; selectGroupByOrderByOrderByLimit -SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages ORDER BY max ASC) ORDER BY max DESC LIMIT 4 +SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages ORDER BY max ASC) ORDER BY max DESC LIMIT 4; From 5c96b30656fa334b23e0b91175f23c56518cddca Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 12 May 2021 19:10:36 +0300 Subject: [PATCH 4/8] extract method --- .../xpack/sql/planner/Verifier.java | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) 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 1b3de049548fa..83a2df5702199 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 @@ -36,23 +36,7 @@ static List verifyMappingPlan(PhysicalPlan plan) { }); }); - 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 (orderBy.get() == null && p instanceof OrderExec) { - orderBy.set(((OrderExec) p).order()); - return; - } - if (hasLimit.get() && orderBy.get() != null && p instanceof OrderExec) { - if (((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")); - } - } - }); + checkForNonCollapsableSubselects(plan, failures); return failures; } @@ -73,4 +57,24 @@ static List verifyExecutingPlan(PhysicalPlan plan) { return failures; } + + 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 (orderBy.get() == null && p instanceof OrderExec) { + orderBy.set(((OrderExec) p).order()); + return; + } + if (hasLimit.get() && orderBy.get() != null && p instanceof OrderExec) { + if (((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")); + } + } + }); + } } From af808e69a360a8c57a1212b4a628301582ff2f18 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 12 May 2021 19:51:11 +0300 Subject: [PATCH 5/8] make query results deterministic --- .../plugin/sql/qa/server/src/main/resources/select.sql-spec | 4 ++-- 1 file changed, 2 insertions(+), 2 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 6d60ef61cfd9a..4902ace98fbe0 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 @@ -148,6 +148,6 @@ SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp LIMIT 10) ORDER BY emp_no) selectGroupByOrderByLimit SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages) ORDER BY max DESC LIMIT 3; selectGroupByLimitOrderByLimit -SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages LIMIT 5) ORDER BY max DESC LIMIT 4; +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 LIMIT 4; +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; From 0b52c6f1a96b8f4d8dcf88d6fc243a7ebd4b5d1e Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Thu, 13 May 2021 07:55:56 +0300 Subject: [PATCH 6/8] fix fqn --- .../elasticsearch/xpack/sql/planner/VerifierErrorTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java index 4050fd652eaa1..013b87b685cc4 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java @@ -11,6 +11,7 @@ import org.elasticsearch.xpack.ql.index.EsIndex; import org.elasticsearch.xpack.ql.index.IndexResolution; import org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer; +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.stats.Metrics; @@ -28,7 +29,7 @@ public class VerifierErrorTests extends ESTestCase { TEST_CFG, new SqlFunctionRegistry(), indexResolution, - new org.elasticsearch.xpack.sql.analysis.analyzer.Verifier(new Metrics()) + new Verifier(new Metrics()) ); private final Planner planner = new Planner(); From bb99b44b27a455efa1c3b342481f556606928d09 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Thu, 13 May 2021 09:42:12 +0300 Subject: [PATCH 7/8] rename test class --- .../sql/planner/{VerifierErrorTests.java => VerifierTests.java} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/{VerifierErrorTests.java => VerifierTests.java} (98%) diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierTests.java similarity index 98% rename from x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java rename to x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierTests.java index 013b87b685cc4..6d320b4bd302e 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierTests.java @@ -19,7 +19,7 @@ import static org.elasticsearch.xpack.sql.SqlTestUtils.TEST_CFG; import static org.elasticsearch.xpack.sql.types.SqlTypesTests.loadMapping; -public class VerifierErrorTests extends ESTestCase { +public class VerifierTests extends ESTestCase { private final SqlParser parser = new SqlParser(); private final IndexResolution indexResolution = IndexResolution.valid( From e8d84f2c2dad7343106e387dc113ec9cf1eb26e7 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Thu, 13 May 2021 16:38:25 +0300 Subject: [PATCH 8/8] address review - fix verification check --- .../server/src/main/resources/select.sql-spec | 4 ++++ .../xpack/sql/planner/Verifier.java | 10 ++++----- .../xpack/sql/planner/VerifierTests.java | 22 ++++++++++++++++++- 3 files changed, 29 insertions(+), 7 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 4902ace98fbe0..c629635864ac8 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 @@ -147,7 +147,11 @@ 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 +SELECT * FROM (SELECT max(salary) AS max, languages FROM test_emp GROUP BY languages ORDER BY max ASC NULLS LAST) ORDER BY max DESC NULLS FIRST LIMIT 4; 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 83a2df5702199..ed5b5d017c682 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 @@ -66,13 +66,11 @@ private static void checkForNonCollapsableSubselects(PhysicalPlan plan, List