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 0BD5A28996 for ; Tue, 12 Mar 2019 10:25:28 -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 PgwnQ-GyFW5H for ; Tue, 12 Mar 2019 10:25:27 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 257A32897F for ; Tue, 12 Mar 2019 10:25:26 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH] sql: forbid different sorting orders in ORDER BY From: "n.pettik" In-Reply-To: <20190311154448.73563-1-ivan.koptelov@tarantool.org> Date: Tue, 12 Mar 2019 17:25:24 +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: Ivan Koptelov > 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. 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. > --- > 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 +++- 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. A bit of code-style refactoring: 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); =20