SQL: Disallow queries with inner LIMIT that cannot be executed in ES #75960

Luegg · 2021-08-02T14:56:00Z

Resolves #75830

This PR only disallows subqueries with LIMIT if they cannot be translated into an semantically equivalent ES query. Thus, queries like SELECT languages FROM (SELECT * FROM test_emp LIMIT 10) that can be flattened correctly are still valid. Compared to disallowing all subqueries with LIMIT, this will only break queries that might return wrong results.

Additionally, this change also uncovered an issue with TOP. Because TOP is part of the querySpecification syntax, the LogicalPlanBuilder creates it as a child of OrderBy nodes. Thus, the query SELECT TOP 10 * FROM test_emp ORDER BY salary used to produce the logical plan

\ OrderBy
 \ Limit
  \ EsRelation[test_emp]

the semnatically correct plan would be

\ Limit
 \ OrderBy
  \ EsRelation[test_emp]

This also had the effect that some queries with both TOP and LIMIT were not rejected by the parser.

elasticmachine · 2021-08-02T15:03:19Z

Pinging @elastic/es-ql (Team:QL)

bpintea · 2021-08-03T08:13:18Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java

+            if (unary.child() instanceof Limit) {
+                Limit limit = (Limit) unary.child();
+
+                return limit.replaceChildrenSameSize(Arrays.asList(


nit: Collections#singletonList() might work too.

bpintea · 2021-08-03T08:20:03Z

x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec

-selectOrderByLimitSameOrderBy
-SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp LIMIT 10) ORDER BY emp_no) ORDER BY emp_no LIMIT 5;


Wondering if this (type of) sequence of sub-selects couldn't be flattened safely. The outer ones don't change the ordering and out of multiple limitations, the smallest value could be determined (there's already code doing this comparison, IIRC?).

I think this particular query cannot be flattened as well because it also first limits and then sorts.

But I think you're right that there is a special case where the outer ORDER BY is redundant and could be eliminated as in SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no LIMIT 10) ORDER BY emp_no. This query should be equivalent to SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no LIMIT 10) which can safely be flattened.

I think the same principle applies for redundant filters as well: SELECT * FROM (SELECT * FROM test_emp WHERE emp_no > 10 LIMIT 10) WHERE emp_no > 10. And maybe also GROUP BYs.

I have the feeling this would best be implemented as an additional optimization though. A rule that eliminates redundant WHERE and ORDER BY.

I just noticed that in this particular case, the query could even be flattened because the order is not specified in the inner query. This would be a somewhat surprising behavior but probably aligned with SQL's "unordered bag of tuples" semantics.

bpintea · 2021-08-03T08:30:25Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java

-                return;
-            }
-            if (p instanceof OrderExec) {
-                if (hasLimit.get() && orderBy.get() != null && ((OrderExec) p).order().equals(orderBy.get()) == false) {


Related to the removed query comment: can't we keep this "pass-through" check? I.e. chaining multiple equal orderings if they aren't combined with other clauses?

costin

Left some comments on improving the complexity of the validation rule and the top parser.

costin · 2021-08-03T12:56:55Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java

@@ -119,6 +124,21 @@ public LogicalPlan visitQueryNoWith(QueryNoWithContext ctx) {
        return plan;
    }

+    private LogicalPlan pullUpTopClauseFromQuerySpecification(LogicalPlan plan) {


I'd prefer a more focused approach in handling top. TOP is handled through visitQuerySpecification while ORDER BY is handled inside queryNoWith hence why one can check if the LIMIT exists in case of an OrderBy and do the switch then - replace the child of the Limit with the new OrderBy which points to the Limit child.

costin · 2021-08-03T12:58:15Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java

+
+        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")


Please order the upper cased keywords alphabetically (for consistency).

costin · 2021-08-03T13:04:08Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/Verifier.java

+        Set<LimitExec> 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) {


No need to do double iteration - the main forEach already traverses the tree so for each matching node doing another p.forEachDown is unnecessary.
Further more I don't think it's useful to the user to see all the instances where the same type of error occurs since a query with ORDER BY and a WHERE clause will report the error twice (once per entity).

I favor the initial approach since it's O(n) - traverse the tree once. (this could be optimized by ending the traversal on the first error however it's a rare enough case that we can ignore it).

👍 for the single iteration.

Also reporting only one error shouldn't make too much of a difference but I was more thinking about the cases where there is more than one offending Limit as in SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp LIMIT 10) LIMIT 20) WHERE languages > 5. Since this seems to be a rather far-fetched case, the simpler one-error implementation should be fine.

Luegg · 2021-08-03T14:48:37Z

@elasticmachine run elasticsearch-ci/bwc

Luegg · 2021-08-03T15:12:41Z

@elasticmachine run elasticsearch-ci/bwc

Luegg · 2021-08-04T08:29:19Z

@costin and @bpintea I've addressed your comments and had a look at what Bogdan mentioned about queries with redundant sort orders (SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no LIMIT 10) ORDER BY emp_no). In theory, we could allow such queries but I'm not sure it's worth it.

The last commit is an attempt to allow these queries and also to address #76013 . Basically, it introduces a PushDownAndCombineOrderBy that folds multiple order bys if possible. The same logic could probably also be implemented in the QueryFolder but to me it looked more like an optimization.

Luegg · 2021-08-04T08:30:44Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

@@ -165,6 +167,9 @@ public LogicalPlan optimize(LogicalPlan verified) {
                new PushDownAndCombineFilters()
        );

+        Batch pushDownOrderBy = new Batch("Push Down Order By",
+            new PushDownAndCombineOrderBy());


Needs to run isolated from PushDownAndCombineFilters in order to avoid interfering push downs.

What's the side effect? Keeping this rule in the main loop has the benefit of picking up plans where some orderby might be removed that would otherwise prevent the pushdown.

I think the optimizer would never converge to a stable plan in some cases as OrderBy and Filter would alternately be pushed down.

Luegg · 2021-08-04T08:33:48Z

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");


I think we should be defensive here because the naive approach to just prepend additional sort orders leads to #76013 .

Alternatively, FoldOrderBy (and Verifier) could take over the responsibility to merge OrderBys from PushDownAndCombineOrderBy.

Alternatively, FoldOrderBy (and Verifier) could take over the responsibility to merge OrderBys from PushDownAndCombineOrderBy.

I like this better.

costin

The last commit is an attempt to allow these queries and also to address #76013 .

Please address that through a different PR (and +1 at enhancing FoldOrderBy instead of a separate rule) to keep things clean.
Otherwise LGTM.

costin · 2021-08-05T10:55:37Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

@@ -165,6 +167,9 @@ public LogicalPlan optimize(LogicalPlan verified) {
                new PushDownAndCombineFilters()
        );

+        Batch pushDownOrderBy = new Batch("Push Down Order By",
+            new PushDownAndCombineOrderBy());


What's the side effect? Keeping this rule in the main loop has the benefit of picking up plans where some orderby might be removed that would otherwise prevent the pushdown.

costin · 2021-08-05T10:57:38Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

+                UnaryPlan child = (UnaryPlan) plan.child();
+                Holder<Boolean> hasAggregate = new Holder<>(false);
+                if (child instanceof Filter) {
+                    child.forEachExpressionDown(e -> hasAggregate.set(hasAggregate.get() || Functions.isAggregate(e)));


This code is compact but it also triggers the set/get for each expression.

if (Function.isAggregate(e)) { hasAggregate.set(true); }

Is more verbose but sane.

costin · 2021-08-05T10:58:03Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

+                }
+
+                if (hasAggregate.get() == false) {
+                    return child.replaceChildrenSameSize(


This could take advantage of the replaceChild method.

costin · 2021-08-05T11:00:56Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

+            return plan;
+        }
+
+        private List<Order> combineOrders(List<Order> parentOrders, List<Order> childOrders) {


It's not clear to me how this method combines orders?

costin · 2021-08-05T11:01:21Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java

+            Source source = source(ctx.ORDER(), endContext);
+            List<Order> order = visitList(ctx.orderBy(), Order.class);
+
+            if (plan instanceof Limit) {


costin · 2021-08-05T11:02:05Z

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");


Alternatively, FoldOrderBy (and Verifier) could take over the responsibility to merge OrderBys from PushDownAndCombineOrderBy.

I like this better.

8d8d992

Luegg · 2021-08-09T06:37:07Z

I've reverted the last commit and will try to address #76013 in a separate PR. This means that we will have a temporary regression as will no longer support queries with redundant sorts (e.g. SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no LIMIT 10) ORDER BY emp_no). Since it seems to be quite a special case, I would say that's justifiable.

…lastic#75960) * Disallow queries with inner limits that cannot be executed correctly * address review comments * introduce PushDownAndCombineOrderBy optimization * Revert "introduce PushDownAndCombineOrderBy optimization" 8d8d992 * use replaceChild

elasticsearchmachine · 2021-08-09T07:53:11Z

💚 Backport successful

Status	Branch	Result
✅	7.14
✅	7.x

…75960) (#76222) * Disallow queries with inner limits that cannot be executed correctly * address review comments * introduce PushDownAndCombineOrderBy optimization * Revert "introduce PushDownAndCombineOrderBy optimization" 8d8d992 * use replaceChild Co-authored-by: Lukas Wegmann <[email protected]>

…75960) (#76223) * Disallow queries with inner limits that cannot be executed correctly * address review comments * introduce PushDownAndCombineOrderBy optimization * Revert "introduce PushDownAndCombineOrderBy optimization" 8d8d992 * use replaceChild Co-authored-by: Lukas Wegmann <[email protected]>

Disallow queries with inner limits that cannot be executed correctly

545be94

elasticsearchmachine added the v8.0.0 label Aug 2, 2021

Luegg changed the title ~~Disallow queries with inner LIMIT that cannot be executed correctly~~ SQL: Disallow queries with inner LIMIT that cannot be executed in ES Aug 2, 2021

Luegg requested review from astefan, bpintea and costin August 2, 2021 15:03

Luegg marked this pull request as ready for review August 2, 2021 15:03

Luegg added the :Analytics/SQL SQL querying label Aug 2, 2021

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Aug 2, 2021

Luegg added >bug v7.15.0 v7.14.1 labels Aug 2, 2021

Merge origin/master into fix/limitInInnerQueries

b3a235a

bpintea reviewed Aug 3, 2021

View reviewed changes

Merge origin/master into fix/limitInInnerQueries

5fa12c7

costin reviewed Aug 3, 2021

View reviewed changes

address review comments

264e042

Luegg force-pushed the fix/limitInInnerQueries branch from 2c263c4 to 264e042 Compare August 3, 2021 14:07

Luegg added 3 commits August 3, 2021 18:01

Merge origin/master into fix/limitInInnerQueries

5cb4017

Merge origin/master into fix/limitInInnerQueries

9f40d30

introduce PushDownAndCombineOrderBy optimization

8d8d992

Luegg commented Aug 4, 2021

View reviewed changes

Luegg requested review from costin and bpintea August 4, 2021 08:35

costin approved these changes Aug 5, 2021

View reviewed changes

Luegg added 2 commits August 9, 2021 08:19

Revert "introduce PushDownAndCombineOrderBy optimization"

a6ab253

8d8d992

Merge origin/master into fix/limitInInnerQueries

226d795

use replaceChild

2edf92d

Luegg merged commit 5e8100b into elastic:master Aug 9, 2021

Luegg deleted the fix/limitInInnerQueries branch August 9, 2021 07:44

Luegg added the auto-backport Automatically create backport pull requests when merged label Aug 9, 2021

elasticsearchmachine mentioned this pull request Aug 9, 2021

[7.14] SQL: Disallow queries with inner LIMIT that cannot be executed in ES (#75960) #76222

Merged

elasticsearchmachine mentioned this pull request Aug 9, 2021

[7.x] SQL: Disallow queries with inner LIMIT that cannot be executed in ES (#75960) #76223

Merged

Luegg mentioned this pull request Aug 10, 2021

SQL: Fix nested ORDER BY #76277

Merged

jakelandis added v8.0.0-alpha2 and removed v8.0.0 labels Sep 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Disallow queries with inner LIMIT that cannot be executed in ES #75960

SQL: Disallow queries with inner LIMIT that cannot be executed in ES #75960

Luegg commented Aug 2, 2021 •

edited

Loading

elasticmachine commented Aug 2, 2021

bpintea Aug 3, 2021

bpintea Aug 3, 2021

Luegg Aug 3, 2021

Luegg Aug 3, 2021

bpintea Aug 3, 2021 •

edited

Loading

costin left a comment

costin Aug 3, 2021

costin Aug 3, 2021

costin Aug 3, 2021

Luegg Aug 3, 2021

Luegg commented Aug 3, 2021

Luegg commented Aug 3, 2021

Luegg commented Aug 4, 2021

Luegg Aug 4, 2021

costin Aug 5, 2021

Luegg Aug 9, 2021

Luegg Aug 4, 2021

costin Aug 5, 2021

costin left a comment

costin Aug 5, 2021

costin Aug 5, 2021

costin Aug 5, 2021

costin Aug 5, 2021

costin Aug 5, 2021

costin Aug 5, 2021

Luegg commented Aug 9, 2021

elasticsearchmachine commented Aug 9, 2021

		selectOrderByLimitSameOrderBy
		SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp LIMIT 10) ORDER BY emp_no) ORDER BY emp_no LIMIT 5;

SQL: Disallow queries with inner LIMIT that cannot be executed in ES #75960

SQL: Disallow queries with inner LIMIT that cannot be executed in ES #75960

Conversation

Luegg commented Aug 2, 2021 • edited Loading

elasticmachine commented Aug 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luegg commented Aug 3, 2021

Luegg commented Aug 3, 2021

Luegg commented Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luegg commented Aug 9, 2021

elasticsearchmachine commented Aug 9, 2021

💚 Backport successful

Luegg commented Aug 2, 2021 •

edited

Loading

bpintea Aug 3, 2021 •

edited

Loading