From: "i.koptelov" <ivan.koptelov@tarantool.org>
To: tarantool-patches@freelists.org
Cc: "n.pettik" <korablev@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH] sql: forbid different sorting orders in ORDER BY
Date: Fri, 15 Mar 2019 15:10:28 +0300 [thread overview]
Message-ID: <A37ADFCC-BDF9-41E5-B696-076B0D24647D@tarantool.org> (raw)
In-Reply-To: <B7B53113-B8D8-40C7-ADF5-0F980196A47B@tarantool.org>
Thank you for the review.
> On 12 Mar 2019, at 17:25, n.pettik <korablev@tarantool.org> wrote:
>
>>
>> On 11 Mar 2019, at 18:44, Ivan Koptelov <ivan.koptelov@tarantool.org> wrote:
>>
>> 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.
>>
>> Close #4038
>>
>> @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.
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) ::= SELECT(S) distinct(D) selcollist(W) from(X) where_opt(Y)
#ifdef SQL_DEBUG
Token s = S; /*A-overwrites-S*/
#endif
+ if (L.pLimit != NULL)
+ sql_expr_check_sort_orders(pParse, Z);
A = sqlSelectNew(pParse,W,X,Y,P,Q,Z,D,L.pLimit,L.pOffset);
Commit message is also fixed.
>
>> ---
>> Branch https://github.com/tarantool/tarantool/tree/sudobobo/gh-4038-ban-diff-orders-in-ORDER-BY
>> Issue https://github.com/tarantool/tarantool/issues/4038
>>
>> 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’t 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 = require("sqltester")
-test:plan(52)
+test:plan(18)
--!./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 = "orderby6"
@@ -150,44 +158,48 @@ testprefix = "orderby6"
-- Many test cases where the LIMIT+OFFSET window is in various
-- alignments with block-sort boundaries.
--
- local data = {
- {limit=0, offset=4, orderby="+b,+a"},
- {limit=0, offset=5, orderby="+b,+a"},
- {limit=0, offset=6, orderby="+b,+a"},
- {limit=0, offset=9, orderby="+b,+a"},
- {limit=0, offset=0, orderby="+b,+a"},
- {limit=0, offset=1, orderby="+b,+a"},
- {limit=7, offset=4, orderby="+b,+a"},
- {limit=7, offset=9, orderby="+b,+a"},
- {limit=0, offset=4, orderby="+b DESC,+a"},
- {limit=0, offset=5, orderby="+b DESC,+a"},
- {limit=0, offset=6, orderby="+b DESC,+a"},
- {limit=0, offset=9, orderby="+b DESC,+a"},
- {limit=0, offset=0, orderby="+b DESC,+a"},
- {limit=0, offset=1, orderby="+b DESC,+a"},
- {limit=7, offset=4, orderby="+b DESC,+a"},
- {limit=7, offset=9, orderby="+b DESC,+a"},
- {limit=0, offset=4, orderby="+b,+a DESC"},
- {limit=0, offset=5, orderby="+b,+a DESC"},
- {limit=0, offset=6, orderby="+b,+a DESC"},
- {limit=0, offset=9, orderby="+b,+a DESC"},
- {limit=0, offset=0, orderby="+b,+a DESC"},
- {limit=0, offset=1, orderby="+b,+a DESC"},
- {limit=7, offset=4, orderby="+b,+a DESC"},
- {limit=7, offset=9, orderby="+b,+a DESC"},
- {limit=0, offset=4, orderby="+b DESC,+a DESC"},
- {limit=0, offset=5, orderby="+b DESC,+a DESC"},
- {limit=0, offset=6, orderby="+b DESC,+a DESC"},
- {limit=0, offset=9, orderby="+b DESC,+a DESC"},
- {limit=0, offset=0, orderby="+b DESC,+a DESC"},
- {limit=0, offset=1, orderby="+b DESC,+a DESC"},
- {limit=7, offset=4, orderby="+b DESC,+a DESC"},
- {limit=7, offset=9, orderby="+b DESC,+a DESC"}}
- for i, v in ipairs(data) do
- local sql1 = "SELECT a FROM t1 ORDER BY "..v.orderby.." LIMIT "..v.limit.." OFFSET "..v.offset..";"
- local sql2 = "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.
+ -- <begin>
+ --local data = {
+ -- {limit=0, offset=4, orderby="+b,+a"},
+ -- {limit=0, offset=5, orderby="+b,+a"},
+ -- {limit=0, offset=6, orderby="+b,+a"},
+ -- {limit=0, offset=9, orderby="+b,+a"},
+ -- {limit=0, offset=0, orderby="+b,+a"},
+ -- {limit=0, offset=1, orderby="+b,+a"},
+ -- {limit=7, offset=4, orderby="+b,+a"},
+ -- {limit=7, offset=9, orderby="+b,+a"},
+ -- {limit=0, offset=4, orderby="+b DESC,+a"},
+ -- {limit=0, offset=5, orderby="+b DESC,+a"},
+ -- {limit=0, offset=6, orderby="+b DESC,+a"},
+ -- {limit=0, offset=9, orderby="+b DESC,+a"},
+ -- {limit=0, offset=0, orderby="+b DESC,+a"},
+ -- {limit=0, offset=1, orderby="+b DESC,+a"},
+ -- {limit=7, offset=4, orderby="+b DESC,+a"},
+ -- {limit=7, offset=9, orderby="+b DESC,+a"},
+ -- {limit=0, offset=4, orderby="+b,+a DESC"},
+ -- {limit=0, offset=5, orderby="+b,+a DESC"},
+ -- {limit=0, offset=6, orderby="+b,+a DESC"},
+ -- {limit=0, offset=9, orderby="+b,+a DESC"},
+ -- {limit=0, offset=0, orderby="+b,+a DESC"},
+ -- {limit=0, offset=1, orderby="+b,+a DESC"},
+ -- {limit=7, offset=4, orderby="+b,+a DESC"},
+ -- {limit=7, offset=9, orderby="+b,+a DESC"},
+ -- {limit=0, offset=4, orderby="+b DESC,+a DESC"},
+ -- {limit=0, offset=5, orderby="+b DESC,+a DESC"},
+ -- {limit=0, offset=6, orderby="+b DESC,+a DESC"},
+ -- {limit=0, offset=9, orderby="+b DESC,+a DESC"},
+ -- {limit=0, offset=0, orderby="+b DESC,+a DESC"},
+ -- {limit=0, offset=1, orderby="+b DESC,+a DESC"},
+ -- {limit=7, offset=4, orderby="+b DESC,+a DESC"},
+ -- {limit=7, offset=9, orderby="+b DESC,+a DESC"}}
+ --for i, v in ipairs(data) do
+ -- local sql1 = "SELECT a FROM t1 ORDER BY "..v.orderby.." LIMIT "..v.limit.." OFFSET "..v.offset..";"
+ -- local sql2 = "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
+ -- <end>
@@ -297,20 +309,23 @@ testprefix = "orderby6"
]],
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;
- ]],
- 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"
- )
+ -- Tests are commented because of the reason described in
+ -- NOTE at the beginning of the file.
+ -- <begin>
+ --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"
+ -- )
+ -- <end>
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 = require("sqltester")
-test:plan(201)
+test:plan(203)
--!./tcltestrunner.lua
-- 2015-01-19
@@ -50,4 +50,29 @@ for i=1,200 do
})
end
+-- 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()
>
> 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)
> }
>
> 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 == NULL)
> return;
> - for (int i = 0; i < expr_list->nExpr; i++) {
> + enum sort_order reference_order = expr_list->a[0].sort_order;
> + for (int i = 1; i < expr_list->nExpr; i++) {
> assert(expr_list->a[i].sort_order != SORT_ORDER_UNDEF);
> - if (expr_list->a[i].sort_order != expr_list->a[0].sort_order) {
> + if (expr_list->a[i].sort_order != 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) ::= . {U = 0;}
>
> orderby_opt(A) ::= . {A = 0;}
> orderby_opt(A) ::= ORDER BY sortlist(X). {
> - sql_check_sort_orders(X, pParse);
> + sql_check_sort_orders(pParse, X);
> A = X;
> }
> sortlist(A) ::= 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);
>
> /**
> - * 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 = sort_order;
}
+void
+sql_expr_check_sort_orders(struct Parse *parse,
+ const struct ExprList *expr_list)
+{
+ if(expr_list == NULL)
+ return;
+ enum sort_order reference_order = expr_list->a[0].sort_order;
+ for (int i = 1; i < expr_list->nExpr; i++) {
+ assert(expr_list->a[i].sort_order != SORT_ORDER_UNDEF);
+ if (expr_list->a[i].sort_order != 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);
+
next prev parent reply other threads:[~2019-03-15 12:10 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-11 15:44 [tarantool-patches] " Ivan Koptelov
2019-03-12 14:25 ` [tarantool-patches] " n.pettik
2019-03-15 12:10 ` i.koptelov [this message]
2019-03-15 16:23 ` n.pettik
2019-03-15 16:52 ` i.koptelov
2019-03-15 16:55 ` n.pettik
2019-03-19 11:25 ` Kirill Yukhin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=A37ADFCC-BDF9-41E5-B696-076B0D24647D@tarantool.org \
--to=ivan.koptelov@tarantool.org \
--cc=korablev@tarantool.org \
--cc=tarantool-patches@freelists.org \
--subject='[tarantool-patches] Re: [PATCH] sql: forbid different sorting orders in ORDER BY' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox