[tarantool-patches] Re: [PATCH] sql: forbid different sorting orders in ORDER BY

["n.pettik" <korablev@tarantool.org>]

> 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.

> ---
> 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.

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