From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id E0E53284B5 for ; Fri, 15 Mar 2019 08:10:32 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id F-zlVmBiQJaP for ; Fri, 15 Mar 2019 08:10:32 -0400 (EDT) Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id D8AC71FA8C for ; Fri, 15 Mar 2019 08:10:31 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH] sql: forbid different sorting orders in ORDER BY From: "i.koptelov" In-Reply-To: Date: Fri, 15 Mar 2019 15:10:28 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20190311154448.73563-1-ivan.koptelov@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: "n.pettik" Thank you for the review. > On 12 Mar 2019, at 17:25, n.pettik wrote: >=20 >>=20 >> On 11 Mar 2019, at 18:44, Ivan Koptelov = wrote: >>=20 >> Without multi-directional iterators (to be introduced in #3309) >> in most cases (except those when internal VDBE merge sorter is >> used) ORDER BY with different specified ordered leads to wrong >> result. So for now (till #3309 is resolved) different sort >> orders are forbidden. >>=20 >> Close #4038 >>=20 >> @TarantoolBot document >> Title: different sorting orders in ORDER BY are forbidden now >> The error will be raised if different sorting orders are >> encountered. >=20 > Since different sorting orders still can be accepted > (for instance, if you remove LIMIT clause from query mentioned > in issue description, it will work fine), mb it is worth to abort > execution only when it comes to sorting by ephemeral space. > Please, ask other members of server team for their opinion > on this case. Pros: different sorting orders will be available > in most cases; cons: this feature will be inconsistent - in some > cases it will work, for other queries it will throw an error. After a short discussion we decided to forbid only ORDER BY + LIMIT + (ASC + DESC). Here is corresponding code changes: diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 996f55d37..e2622222e 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -458,6 +458,8 @@ oneselect(A) ::=3D SELECT(S) distinct(D) = selcollist(W) from(X) where_opt(Y) #ifdef SQL_DEBUG Token s =3D S; /*A-overwrites-S*/ #endif + if (L.pLimit !=3D NULL) + sql_expr_check_sort_orders(pParse, Z); A =3D sqlSelectNew(pParse,W,X,Y,P,Q,Z,D,L.pLimit,L.pOffset); Commit message is also fixed. >=20 >> --- >> Branch = https://github.com/tarantool/tarantool/tree/sudobobo/gh-4038-ban-diff-orde= rs-in-ORDER-BY >> Issue https://github.com/tarantool/tarantool/issues/4038 >>=20 >> src/box/sql/expr.c | 15 +++ >> src/box/sql/parse.y | 5 +- >> src/box/sql/sqlInt.h | 14 +++ >> test/sql-tap/e_select1.test.lua | 165 = ++++++++++++++++++--------- >> test/sql-tap/orderby1.test.lua | 67 ++++++----- >> test/sql-tap/orderby2.test.lua | 15 ++- >> test/sql-tap/orderby6.test.lua | 125 ++++++++++++-------- >> test/sql-tap/select1.test.lua | 15 ++- >> test/sql-tap/select4.test.lua | 3 +- >> test/sql-tap/sort.test.lua | 35 +++--- >> test/sql-tap/tkt-4dd95f6943.test.lua | 128 ++++++++++++--------- >> test/sql-tap/tkt-b75a9ca6b0.test.lua | 33 +++--- >> test/sql-tap/tkt-ba7cbfaedc.test.lua | 24 +++- >> test/sql-tap/where2.test.lua | 20 +++- >=20 > Plase, make up a list of tests to be enabled after multi-directional > iterators are introduced. Don=E2=80=99t fix tests which fail now, = simply comment > them. Finally, add a few simple tests verifying that different sorting > orders are not supported now. I did as you say. Added list of tests as a comment to #3309. But I would like to know, why commenting tests & making a list of them is better than change tests & comment old expected results? The main advantage is that one can not ignore/miss tests, which would fail after #3309 is done. diff --git a/test/sql-tap/orderby6.test.lua = b/test/sql-tap/orderby6.test.lua index 213e87421..7fe0032dd 100755 --- a/test/sql-tap/orderby6.test.lua +++ b/test/sql-tap/orderby6.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test =3D require("sqltester") -test:plan(52) +test:plan(18) =20 --!./tcltestrunner.lua -- 2014-03-21 @@ -16,6 +16,14 @@ test:plan(52) -- This file implements regression tests for sql library. The -- focus of this file is testing that the block-sort optimization. -- +-- NOTE: tests with ORDER BY + LIMIT + (ASC + DESC, i.e +-- different sorting orders) are commented in this file as they +-- are failing because we don't have multi-directional iterators. +-- Details and full list of commented tests can be found here: +-- https://github.com/tarantool/tarantool/issues/3309 +-- Please, uncomment the tests when multi-directional iterators +-- are introduced. +-- -- ["set","testdir",[["file","dirname",["argv0"]]]] -- ["source",[["testdir"],"\/tester.tcl"]] testprefix =3D "orderby6" @@ -150,44 +158,48 @@ testprefix =3D "orderby6" -- Many test cases where the LIMIT+OFFSET window is in various -- alignments with block-sort boundaries. -- - local data =3D { - {limit=3D0, offset=3D4, orderby=3D"+b,+a"}, - {limit=3D0, offset=3D5, orderby=3D"+b,+a"}, - {limit=3D0, offset=3D6, orderby=3D"+b,+a"}, - {limit=3D0, offset=3D9, orderby=3D"+b,+a"}, - {limit=3D0, offset=3D0, orderby=3D"+b,+a"}, - {limit=3D0, offset=3D1, orderby=3D"+b,+a"}, - {limit=3D7, offset=3D4, orderby=3D"+b,+a"}, - {limit=3D7, offset=3D9, orderby=3D"+b,+a"}, - {limit=3D0, offset=3D4, orderby=3D"+b DESC,+a"}, - {limit=3D0, offset=3D5, orderby=3D"+b DESC,+a"}, - {limit=3D0, offset=3D6, orderby=3D"+b DESC,+a"}, - {limit=3D0, offset=3D9, orderby=3D"+b DESC,+a"}, - {limit=3D0, offset=3D0, orderby=3D"+b DESC,+a"}, - {limit=3D0, offset=3D1, orderby=3D"+b DESC,+a"}, - {limit=3D7, offset=3D4, orderby=3D"+b DESC,+a"}, - {limit=3D7, offset=3D9, orderby=3D"+b DESC,+a"}, - {limit=3D0, offset=3D4, orderby=3D"+b,+a DESC"}, - {limit=3D0, offset=3D5, orderby=3D"+b,+a DESC"}, - {limit=3D0, offset=3D6, orderby=3D"+b,+a DESC"}, - {limit=3D0, offset=3D9, orderby=3D"+b,+a DESC"}, - {limit=3D0, offset=3D0, orderby=3D"+b,+a DESC"}, - {limit=3D0, offset=3D1, orderby=3D"+b,+a DESC"}, - {limit=3D7, offset=3D4, orderby=3D"+b,+a DESC"}, - {limit=3D7, offset=3D9, orderby=3D"+b,+a DESC"}, - {limit=3D0, offset=3D4, orderby=3D"+b DESC,+a DESC"}, - {limit=3D0, offset=3D5, orderby=3D"+b DESC,+a DESC"}, - {limit=3D0, offset=3D6, orderby=3D"+b DESC,+a DESC"}, - {limit=3D0, offset=3D9, orderby=3D"+b DESC,+a DESC"}, - {limit=3D0, offset=3D0, orderby=3D"+b DESC,+a DESC"}, - {limit=3D0, offset=3D1, orderby=3D"+b DESC,+a DESC"}, - {limit=3D7, offset=3D4, orderby=3D"+b DESC,+a DESC"}, - {limit=3D7, offset=3D9, orderby=3D"+b DESC,+a DESC"}} - for i, v in ipairs(data) do - local sql1 =3D "SELECT a FROM t1 ORDER BY "..v.orderby.." LIMIT = "..v.limit.." OFFSET "..v.offset..";" - local sql2 =3D "SELECT a FROM t1 ORDER BY = "..string.gsub(v.orderby, "+", "").." LIMIT "..v.limit.." OFFSET = "..v.offset..";" - test:do_execsql_test("1.21."..i, sql1, test:execsql(sql2)) - end + -- Tests are commented because of the reason described in + -- NOTE at the beginning of the file. + -- + --local data =3D { + -- {limit=3D0, offset=3D4, orderby=3D"+b,+a"}, + -- {limit=3D0, offset=3D5, orderby=3D"+b,+a"}, + -- {limit=3D0, offset=3D6, orderby=3D"+b,+a"}, + -- {limit=3D0, offset=3D9, orderby=3D"+b,+a"}, + -- {limit=3D0, offset=3D0, orderby=3D"+b,+a"}, + -- {limit=3D0, offset=3D1, orderby=3D"+b,+a"}, + -- {limit=3D7, offset=3D4, orderby=3D"+b,+a"}, + -- {limit=3D7, offset=3D9, orderby=3D"+b,+a"}, + -- {limit=3D0, offset=3D4, orderby=3D"+b DESC,+a"}, + -- {limit=3D0, offset=3D5, orderby=3D"+b DESC,+a"}, + -- {limit=3D0, offset=3D6, orderby=3D"+b DESC,+a"}, + -- {limit=3D0, offset=3D9, orderby=3D"+b DESC,+a"}, + -- {limit=3D0, offset=3D0, orderby=3D"+b DESC,+a"}, + -- {limit=3D0, offset=3D1, orderby=3D"+b DESC,+a"}, + -- {limit=3D7, offset=3D4, orderby=3D"+b DESC,+a"}, + -- {limit=3D7, offset=3D9, orderby=3D"+b DESC,+a"}, + -- {limit=3D0, offset=3D4, orderby=3D"+b,+a DESC"}, + -- {limit=3D0, offset=3D5, orderby=3D"+b,+a DESC"}, + -- {limit=3D0, offset=3D6, orderby=3D"+b,+a DESC"}, + -- {limit=3D0, offset=3D9, orderby=3D"+b,+a DESC"}, + -- {limit=3D0, offset=3D0, orderby=3D"+b,+a DESC"}, + -- {limit=3D0, offset=3D1, orderby=3D"+b,+a DESC"}, + -- {limit=3D7, offset=3D4, orderby=3D"+b,+a DESC"}, + -- {limit=3D7, offset=3D9, orderby=3D"+b,+a DESC"}, + -- {limit=3D0, offset=3D4, orderby=3D"+b DESC,+a DESC"}, + -- {limit=3D0, offset=3D5, orderby=3D"+b DESC,+a DESC"}, + -- {limit=3D0, offset=3D6, orderby=3D"+b DESC,+a DESC"}, + -- {limit=3D0, offset=3D9, orderby=3D"+b DESC,+a DESC"}, + -- {limit=3D0, offset=3D0, orderby=3D"+b DESC,+a DESC"}, + -- {limit=3D0, offset=3D1, orderby=3D"+b DESC,+a DESC"}, + -- {limit=3D7, offset=3D4, orderby=3D"+b DESC,+a DESC"}, + -- {limit=3D7, offset=3D9, orderby=3D"+b DESC,+a DESC"}} + --for i, v in ipairs(data) do + -- local sql1 =3D "SELECT a FROM t1 ORDER BY "..v.orderby.." = LIMIT "..v.limit.." OFFSET "..v.offset..";" + -- local sql2 =3D "SELECT a FROM t1 ORDER BY = "..string.gsub(v.orderby, "+", "").." LIMIT "..v.limit.." OFFSET = "..v.offset..";" + -- test:do_execsql_test("1.21."..i, sql1, test:execsql(sql2)) + --end + -- =20 @@ -297,20 +309,23 @@ testprefix =3D "orderby6" ]],=20 test:execsql "SELECT a FROM t2 ORDER BY +b DESC,+c,+d,+e,+f;" ) - - test:do_execsql_test( - "1.42", - [[ - SELECT a FROM t2 ORDER BY b DESC,c DESC,d,e,f LIMIT 31; - ]],=20 - test:execsql "SELECT a FROM t2 ORDER BY +b DESC,+c = DESC,+d,+e,+f LIMIT 31" - ) - - test:do_execsql_test( - "1.43", - [[ - SELECT a FROM t2 ORDER BY b,c,d,e,f DESC LIMIT 8 OFFSET 7; - ]],=20 - test:execsql "SELECT a FROM t2 ORDER BY +b,+c,+d,+e,+f DESC = LIMIT 8 OFFSET 7" - ) + -- Tests are commented because of the reason described in + -- NOTE at the beginning of the file. + -- + --test:do_execsql_test( + -- "1.42", + -- [[ + -- SELECT a FROM t2 ORDER BY b DESC,c DESC,d,e,f LIMIT 31; + -- ]], + -- test:execsql "SELECT a FROM t2 ORDER BY +b DESC,+c = DESC,+d,+e,+f LIMIT 31" + -- ) + -- + --test:do_execsql_test( + -- "1.43", + -- [[ + -- SELECT a FROM t2 ORDER BY b,c,d,e,f DESC LIMIT 8 OFFSET = 7; + -- ]], + -- test:execsql "SELECT a FROM t2 ORDER BY +b,+c,+d,+e,+f DESC = LIMIT 8 OFFSET 7" + -- ) + -- test:finish_test() diff --git a/test/sql-tap/orderby8.test.lua = b/test/sql-tap/orderby8.test.lua index 92fec6dbe..95acee1e1 100755 --- a/test/sql-tap/orderby8.test.lua +++ b/test/sql-tap/orderby8.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test =3D require("sqltester") -test:plan(201) +test:plan(203) =20 --!./tcltestrunner.lua -- 2015-01-19 @@ -50,4 +50,29 @@ for i=3D1,200 do }) end =20 +-- Tests to check that ORDER BY + LIMIT + (ASC + DESC, different +-- sorting orders) are forbidden. Such combination is forbidden +-- because currently it returns data in wrong sorting order. +-- It will be fixed when multi-directional would be introduced: +-- https://github.com/tarantool/tarantool/issues/3309 + +test:do_catchsql_test( + "1.201", + [[ + CREATE TABLE t2 (id INT PRIMARY KEY, a INT, b INT); + INSERT INTO t2 VALUES (1, 2, 1), (2, -3, 5), (3, 2, -3), (4, 2, = 12); + SELECT * FROM t2 ORDER BY a ASC, b DESC LIMIT 5; + ]], + {1, "ORDER BY does not support different sorting orders"} +) + +test:do_catchsql_test( + "1.202", + [[ + SELECT * FROM t2 ORDER BY a, b DESC LIMIT 5; + ]], + {1, "ORDER BY does not support different sorting orders"} + +) + test:finish_test() >=20 > A bit of code-style refactoring: >=20 > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 85719e3a1..bd494ef2e 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -1810,12 +1810,15 @@ sqlExprListSetSortOrder(struct ExprList *p, = enum sort_order sort_order) > } >=20 > void > -sql_check_sort_orders(ExprList * expr_list, Parse *parse) { > +sql_expr_check_sort_orders(struct Parse *parse, > + const struct ExprList *expr_list) > +{ > if(expr_list =3D=3D NULL) > return; > - for (int i =3D 0; i < expr_list->nExpr; i++) { > + enum sort_order reference_order =3D = expr_list->a[0].sort_order; > + for (int i =3D 1; i < expr_list->nExpr; i++) { > assert(expr_list->a[i].sort_order !=3D = SORT_ORDER_UNDEF); > - if (expr_list->a[i].sort_order !=3D = expr_list->a[0].sort_order) { > + if (expr_list->a[i].sort_order !=3D reference_order) { > diag_set(ClientError, ER_UNSUPPORTED, "ORDER = BY", > "different sorting orders"); > sql_parser_error(parse); > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index 95d6962f9..f056a89b4 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -662,7 +662,7 @@ using_opt(U) ::=3D . {U =3D = 0;} >=20 > orderby_opt(A) ::=3D . {A =3D 0;} > orderby_opt(A) ::=3D ORDER BY sortlist(X). { > - sql_check_sort_orders(X, pParse); > + sql_check_sort_orders(pParse, X); > A =3D X; > } > sortlist(A) ::=3D sortlist(A) COMMA expr(Y) sortorder(Z). { > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 06818d9e1..324ab1618 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -3238,18 +3238,19 @@ ExprList *sqlExprListAppendVector(Parse *, = ExprList *, IdList *, Expr *); > void sqlExprListSetSortOrder(ExprList *, enum sort_order sort_order); >=20 > /** > - * Check if sorting orders are the same in ORDER BY and rise an > + * Check if sorting orders are the same in ORDER BY and raise an > * error if they are not. > * > * In future, we will support different sorting orders in > * ORDER BY (e.g. ORDER BY col1 ASC, col2 DESC) and remove this > - * check (see ticket #3309). > - * @param expr_list Expression list with ORDER BY clause > + * check. > + * * @param parse Parsing context. > + * @param expr_list Expression list with ORDER BY clause > * at the end. > - * @param parse Parsing context. > */ > void > -sql_check_sort_orders(ExprList * expr_list, Parse *parse); > +sql_expr_check_sort_orders(struct Parse *parse, > + const struct ExprList *expr_list); Thank you! Sorry that you had to do these fixes. Applied all of them. diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index a75f23756..bd494ef2e 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -1809,6 +1809,24 @@ sqlExprListSetSortOrder(struct ExprList *p, enum = sort_order sort_order) p->a[p->nExpr - 1].sort_order =3D sort_order; } =20 +void +sql_expr_check_sort_orders(struct Parse *parse, + const struct ExprList *expr_list) +{ + if(expr_list =3D=3D NULL) + return; + enum sort_order reference_order =3D expr_list->a[0].sort_order; + for (int i =3D 1; i < expr_list->nExpr; i++) { + assert(expr_list->a[i].sort_order !=3D = SORT_ORDER_UNDEF); + if (expr_list->a[i].sort_order !=3D reference_order) { + diag_set(ClientError, ER_UNSUPPORTED, "ORDER = BY", + "different sorting orders"); + sql_parser_error(parse); + return; + } + } +} + +/** + * Check if sorting orders are the same in ORDER BY and raise an + * error if they are not. + * + * This check is needed only for ORDER BY + LIMIT, because + * currently ORDER BY + LIMIT + ASC + DESC produces incorrectly + * sorted results and thus forbidden. In future, we will + * support different sorting orders in + * ORDER BY + LIMIT (e.g. ORDER BY col1 ASC, col2 DESC LIMIT ...) + * and remove this check. + * @param parse Parsing context. + * @param expr_list Expression list with ORDER BY clause + * at the end. + */ +void +sql_expr_check_sort_orders(struct Parse *parse, + const struct ExprList *expr_list); +