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

matriv · 2021-05-12T14:50:01Z

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

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: elastic#71158

elasticmachine · 2021-05-12T16:51:33Z

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

costin

LGTM

costin · 2021-05-12T19:37:55Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java

+import static org.elasticsearch.xpack.sql.SqlTestUtils.TEST_CFG;
+import static org.elasticsearch.xpack.sql.types.SqlTypesTests.loadMapping;
+
+public class VerifierErrorTests extends ESTestCase {


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.

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

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

costin · 2021-05-12T19:38:22Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierErrorTests.java

+        TEST_CFG,
+        new SqlFunctionRegistry(),
+        indexResolution,
+        new org.elasticsearch.xpack.sql.analysis.analyzer.Verifier(new Metrics())


What's with the fqn?

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.

bpintea

lgtm.

bpintea · 2021-05-13T08:13:54Z

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

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


Optional compacting.

Suggested change

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

}

}

if (p instanceof LimitExec) {

if (hasLimit.get() == false) {

hasLimit.set(true); // I'd use either objects or primitives for this and above statement

}

} else if (p instanceof OrderExec) {

if (orderBy.get() == null) {

orderBy.set(((OrderExec) p).order());

} else if (hasLimit.get()) {

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

}

}

}

bpintea · 2021-05-13T08:15:21Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/VerifierTests.java

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


Is the string split meant to emphasise the subselect?

Yep, but the formatter made it an one-liner, will fix.

bpintea · 2021-05-13T08:43:13Z

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

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


Is the error message descriptive enough, tho? That is actually possible[*], just not in any (i.e. "conflicting") configuration, right? Subselects are a gray area anyways, so I guess this could also remain like this, but alternatively we could add some notes in the limitations docs or somehow rephrase this message -- up to you.

[*] Didn't run it on the new code, but I guess this should work: SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no LIMIT 10) ORDER BY emp_no LIMIT 5

Yep it passes, since it's the exact same order By so it can be executed once and then merge the limits to 5: https://github.com/elastic/elasticsearch/pull/72991/files#diff-147ac28e10bf6b2342a276203e3b250ccd593163c3aa087ee6d72ed19572c163R147

Regarding the error message, imho we shouldn't add more details, about not supporting an ORDER BY on top of the result set returned internally, I think it will just complicate things, and regarding the limitations we already state about supporting queries that can be "flattened".

astefan

I am not sure that I fully understand the scenario that we are avoiding here...
You compare the ORDER BYs basically from two different plans that contain each other using if (((OrderExec) p).order().equals(orderBy.get()) == false). The equals method of Order covers the direction (ASC/DESC), on what the ordering is applied (the expression itself) and the nulls position.
Two comments:

NULLS are not covered by the tests. There is only one passing test using NULLS FIRST.
I don't understand why this test passes SELECT * FROM (SELECT * FROM (SELECT * FROM (SELECT * FROM test_emp ORDER BY emp_no DESC) ORDER BY emp_no ASC NULLS LAST) ORDER BY emp_no DESC NULLS LAST LIMIT 12) ORDER BY emp_no DESC NULLS FIRST if the nulls position is different in the outermost query and the first inner query. I must be missing the exact scenario that we are forbidding.

matriv · 2021-05-13T13:43:27Z

@astefan Thx a lot for catching this extra case, in fact when everything inside the hierarchy is flattened the actual ORDER BY applied, is the last one (top of hierarchy) before encountering the next ORDER BY that will break things (because a LIMIT is also already present).

Added more tests for verifying those un-supported queries and a couple more integ tests that also include nulls.

astefan

LGTM

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: elastic#71158 (cherry picked from commit a5a20ae)

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 (cherry picked from commit a5a20ae)

matriv added >bug :Analytics/SQL SQL querying v8.0.0 v7.12.2 v7.14.0 v7.13.1 labels May 12, 2021

matriv added 4 commits May 12, 2021 18:27

unused import

d46d543

fix test

676cb9b

extract method

5c96b30

make query results deterministic

af808e6

matriv requested review from costin, astefan and bpintea May 12, 2021 16:51

matriv marked this pull request as ready for review May 12, 2021 16:51

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

costin approved these changes May 12, 2021

View reviewed changes

matriv added 2 commits May 13, 2021 07:55

fix fqn

0b52c6f

rename test class

bb99b44

bpintea approved these changes May 13, 2021

View reviewed changes

astefan reviewed May 13, 2021

View reviewed changes

address review - fix verification check

e8d84f2

matriv requested a review from astefan May 13, 2021 13:43

astefan approved these changes May 14, 2021

View reviewed changes

matriv merged commit a5a20ae into elastic:master May 14, 2021

matriv deleted the fix-71158 branch May 14, 2021 13:00

mark-vieira added v7.13.0 and removed v7.13.1 labels May 14, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

matriv commented May 12, 2021

elasticmachine commented May 12, 2021

costin left a comment

costin May 12, 2021

matriv May 13, 2021

costin May 13, 2021

costin May 12, 2021

matriv May 13, 2021

bpintea left a comment

bpintea May 13, 2021

bpintea May 13, 2021

matriv May 13, 2021

bpintea May 13, 2021

matriv May 13, 2021

matriv May 13, 2021

astefan left a comment

matriv commented May 13, 2021

astefan left a comment

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

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

Conversation

matriv commented May 12, 2021

elasticmachine commented May 12, 2021

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

bpintea 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

astefan left a comment

Choose a reason for hiding this comment

matriv commented May 13, 2021

astefan left a comment

Choose a reason for hiding this comment