Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL: Disallow non-collapsable subselects with ORDER BY #72991

Merged
merged 8 commits into from
May 14, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
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
  • Loading branch information
matriv committed May 12, 2021
commit f7a89ed5b1c14fdc037e19d8409e0dba7e4dff09
25 changes: 25 additions & 0 deletions x-pack/plugin/sql/qa/server/src/main/resources/select.sql-spec
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -32,6 +36,24 @@ static List<Failure> verifyMappingPlan(PhysicalPlan plan) {
});
});

Holder<Boolean> hasLimit = new Holder<>(Boolean.FALSE);
Holder<List<Order>> 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;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently errors are handled also inside VerifierTests through the error method (iirc); the reason being the test checks both things that pass and those that do not and it's easier to see both in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VerifierTests is in eql, should I just rename it VerifierTests also for the sql class?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say so yes, easier to find it since the difference is just in the packages.


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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's with the fqn?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was automatic from IDE, since we are in the sql.planner package, to avoid mixing with that Verifier (which we actually test here), but will change.

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