[tarantool-patches] [PATCH] sql: remove tests on ORDER BY/LIMIT + DELETE

[tarantool-patches] [PATCH] sql: remove tests on ORDER BY/LIMIT + DELETE

[Ivan Koptelov]

Removes obsolete tests on ORDER BY/LIMIT + DELETE. This
functionality was not supported for some time, but a few tests
were still existing.

Closes #2172
---
Branch https://github.com/tarantool/tarantool/compare/sudobobo/gh-2172-rm-order-by-and-limit-from-delete
Issue https://github.com/tarantool/tarantool/issues/2172

 test/sql-tap/e_delete.test.lua | 188 -----------------------------------------
 1 file changed, 188 deletions(-)

diff --git a/test/sql-tap/e_delete.test.lua b/test/sql-tap/e_delete.test.lua
index 84a4e0a22..0a81eb3bd 100755
--- a/test/sql-tap/e_delete.test.lua
+++ b/test/sql-tap/e_delete.test.lua
@@ -305,194 +305,6 @@ if (0 > 0) then
          DELETE FROM t8 NOT INDEXED WHERE a=5;
        END;]], {"NOT INDEXED"}},
     })
-    -- EVIDENCE-OF: R-64942-06615 The LIMIT and ORDER BY clauses (described
-    -- below) are unsupported for DELETE statements within triggers.
-    --
-    test:do_delete_tests("e_delete-2.5", "-error", [[ near "%s": syntax error ]], {
-     {1, [[
-       CREATE TRIGGER tr3 AFTER INSERT ON t8 BEGIN
-         DELETE FROM t8 LIMIT 10;
-       END;]], {"LIMIT"}},
-     {2, [[
-       CREATE TRIGGER tr3 AFTER INSERT ON t8 BEGIN
-         DELETE FROM t8 ORDER BY a LIMIT 5;
-       END;]], {"ORDER"}}
-   })
-    -- EVIDENCE-OF: R-40026-10531 If SQLite is compiled with the
-    -- SQLITE_ENABLE_UPDATE_DELETE_LIMIT compile-time option, then the syntax
-    -- of the DELETE statement is extended by the addition of optional ORDER
-    -- BY and LIMIT clauses:
-    --
-    -- -- syntax diagram delete-stmt-limited
-    --
-    test:do_delete_tests("e_delete-3.1", {
-       {1, "DELETE FROM t1 LIMIT 5", {}},
-       {2, "DELETE FROM t1 LIMIT 5-1 OFFSET 2+2", {}},
-       {3, "DELETE FROM t1 LIMIT 2+2, 16/4", {}},
-       {4, "DELETE FROM t1 ORDER BY x LIMIT 5", {}},
-       {5, "DELETE FROM t1 ORDER BY x LIMIT 5-1 OFFSET 2+2", {}},
-       {6, "DELETE FROM t1 ORDER BY x LIMIT 2+2, 16/4", {}},
-       {7, "DELETE FROM t1 WHERE x>2 LIMIT 5", {}},
-       {8, "DELETE FROM t1 WHERE x>2 LIMIT 5-1 OFFSET 2+2", {}},
-       {9, "DELETE FROM t1 WHERE x>2 LIMIT 2+2, 16/4", {}},
-       {10, "DELETE FROM t1 WHERE x>2 ORDER BY x LIMIT 5", {}},
-       {11, "DELETE FROM t1 WHERE x>2 ORDER BY x LIMIT 5-1 OFFSET 2+2", {}},
-       {12, "DELETE FROM t1 WHERE x>2 ORDER BY x LIMIT 2+2, 16/4", {}},
-   })
-end
-
-
-
--- MUST_WORK_TEST delete limit syntax
-if (0 > 0) then
-    test.drop_all_tables()
-    local function rebuild_t1()
-        test:catchsql " DROP TABLE t1 "
-        test:execsql [[
-        CREATE TABLE t1(a  INT PRIMARY KEY, b INT );
-        INSERT INTO t1 VALUES(1, 'one');
-        INSERT INTO t1 VALUES(2, 'two');
-        INSERT INTO t1 VALUES(3, 'three');
-        INSERT INTO t1 VALUES(4, 'four');
-        INSERT INTO t1 VALUES(5, 'five');
-    ]]
-    end
-    -- EVIDENCE-OF: R-44062-08550 If a DELETE statement has a LIMIT clause,
-    -- the maximum number of rows that will be deleted is found by evaluating
-    -- the accompanying expression and casting it to an integer value.
-    --
-    rebuild_t1()
-    --test:do_delete_tests("e_delete-3.2", "-repair", "rebuild_t1", "-query", "SELECT a FROM t1", {
-    test:do_delete_tests("e_delete-3.2", {
-   {1, "DELETE FROM t1 LIMIT 3", {4, 5}},
-   {2, "DELETE FROM t1 LIMIT 1+1", {3, 4, 5}},
-   {3, "DELETE FROM t1 LIMIT '4'", {5}},
-   {4, "DELETE FROM t1 LIMIT '1.0'", {2, 3, 4, 5}},
-    })
-    -- EVIDENCE-OF: R-02661-56399 If the result of the evaluating the LIMIT
-    -- clause cannot be losslessly converted to an integer value, it is an
-    -- error.
-    --
-    test:do_delete_tests("e_delete-3.3", "-error", " datatype mismatch ", {
-   {1, "DELETE FROM t1 LIMIT 'abc'", {}},
-   {2, "DELETE FROM t1 LIMIT NULL", {}},
-   {3, "DELETE FROM t1 LIMIT X'ABCD'", {}},
-   {4, "DELETE FROM t1 LIMIT 1.2", {}},
-})
-    -- EVIDENCE-OF: R-00598-03741 A negative LIMIT value is interpreted as
-    -- "no limit".
-    --
-    test:do_delete_tests("e_delete-3.4", "-repair", "rebuild_t1", "-query", [[
- SELECT a FROM t1
-]], {
-   {1, "DELETE FROM t1 LIMIT -1", {}},
-   {2, "DELETE FROM t1 LIMIT 2-4", {}},
-   {3, "DELETE FROM t1 LIMIT -4.0", {}},
-   {4, "DELETE FROM t1 LIMIT 5*-1", {}},
-})
-    -- EVIDENCE-OF: R-26377-49195 If the DELETE statement also has an OFFSET
-    -- clause, then it is similarly evaluated and cast to an integer value.
-    -- Again, it is an error if the value cannot be losslessly converted to
-    -- an integer.
-    --
-    test:do_delete_tests("e_delete-3.5", "-error", " datatype mismatch ", {
-   {1, "DELETE FROM t1 LIMIT 1 OFFSET 'abc'", {}},
-   {2, "DELETE FROM t1 LIMIT 1 OFFSET NULL", {}},
-   {3, "DELETE FROM t1 LIMIT 1 OFFSET X'ABCD'", {}},
-   {4, "DELETE FROM t1 LIMIT 1 OFFSET 1.2", {}},
-   {5, "DELETE FROM t1 LIMIT 'abc', 1", {}},
-   {6, "DELETE FROM t1 LIMIT NULL, 1", {}},
-   {7, "DELETE FROM t1 LIMIT X'ABCD', 1", {}},
-   {8, "DELETE FROM t1 LIMIT 1.2, 1", {}},
-})
-    -- EVIDENCE-OF: R-64004-53814 If there is no OFFSET clause, or the
-    -- calculated integer value is negative, the effective OFFSET value is
-    -- zero.
-    --
-    test:do_delete_tests("e_delete-3.6", "-repair", "rebuild_t1", "-query", [[
- SELECT a FROM t1
-]], {
-{"1a", "DELETE FROM t1 LIMIT 3 OFFSET 0", {4, 5}},
-{"1b", "DELETE FROM t1 LIMIT 3", {4, 5}},
-{"1c", "DELETE FROM t1 LIMIT 3 OFFSET -1", {4, 5}},
-{"2a", "DELETE FROM t1 LIMIT 1+1 OFFSET 0", {3, 4, 5}},
-{"2b", "DELETE FROM t1 LIMIT 1+1", {3, 4, 5}},
-{"2c", "DELETE FROM t1 LIMIT 1+1 OFFSET 2-5", {3, 4, 5}},
-{"3a", "DELETE FROM t1 LIMIT '4' OFFSET 0", {5}},
-{"3b", "DELETE FROM t1 LIMIT '4'", {5}},
-{"3c", "DELETE FROM t1 LIMIT '4' OFFSET -1.0", {5}},
-{"4a", "DELETE FROM t1 LIMIT '1.0' OFFSET 0", {2, 3, 4, 5}},
-{"4b", "DELETE FROM t1 LIMIT '1.0'", {2, 3, 4, 5}},
-{"4c", "DELETE FROM t1 LIMIT '1.0' OFFSET -11", {2, 3, 4, 5}},
-})
-    -- EVIDENCE-OF: R-48141-52334 If the DELETE statement has an ORDER BY
-    -- clause, then all rows that would be deleted in the absence of the
-    -- LIMIT clause are sorted according to the ORDER BY. The first M rows,
-    -- where M is the value found by evaluating the OFFSET clause expression,
-    -- are skipped, and the following N, where N is the value of the LIMIT
-    -- expression, are deleted.
-    --
-    --test:do_delete_tests("e_delete-3.7", "-repair", "rebuild_t1", "-query", "SELECT a FROM t1", {
-    test:do_delete_tests("e_delete-3.7",{
-   {1, "DELETE FROM t1 ORDER BY b LIMIT 2", {1, 2, 3}},
-   {2, "DELETE FROM t1 ORDER BY length(b), a LIMIT 3", {3, 5}},
-   {3, "DELETE FROM t1 ORDER BY a DESC LIMIT 1 OFFSET 0", {1, 2, 3, 4}},
-   {4, "DELETE FROM t1 ORDER BY a DESC LIMIT 1 OFFSET 1", {1, 2, 3, 5}},
-   {5, "DELETE FROM t1 ORDER BY a DESC LIMIT 1 OFFSET 2", {1, 2, 4, 5}},
-})
-    -- EVIDENCE-OF: R-64535-08414 If there are less than N rows remaining
-    -- after taking the OFFSET clause into account, or if the LIMIT clause
-    -- evaluated to a negative value, then all remaining rows are deleted.
-    --
-    test:do_delete_tests("e_delete-3.8", "-repair", "rebuild_t1", "-query", [[
- SELECT a FROM t1
-]], {
-   {1, "DELETE FROM t1 ORDER BY a ASC LIMIT 10", {}},
-   {2, "DELETE FROM t1 ORDER BY a ASC LIMIT -1", {}},
-   {3, "DELETE FROM t1 ORDER BY a ASC LIMIT 4 OFFSET 2", {1, 2}},
-})
-    -- EVIDENCE-OF: R-37284-06965 If the DELETE statement has no ORDER BY
-    -- clause, then all rows that would be deleted in the absence of the
-    -- LIMIT clause are assembled in an arbitrary order before applying the
-    -- LIMIT and OFFSET clauses to determine the subset that are actually
-    -- deleted.
-    --
-    --     In practice, the "arbitrary order" is rowid order.
-    --
-    test:do_delete_tests("e_delete-3.9", "-repair", "rebuild_t1", "-query", [[
- SELECT a FROM t1
-]], {
-   {1, "DELETE FROM t1 LIMIT 2", {3, 4, 5}},
-   {2, "DELETE FROM t1 LIMIT 3", {4, 5}},
-   {3, "DELETE FROM t1 LIMIT 1 OFFSET 0", {2, 3, 4, 5}},
-   {4, "DELETE FROM t1 LIMIT 1 OFFSET 1", {1, 3, 4, 5}},
-   {5, "DELETE FROM t1 LIMIT 1 OFFSET 2", {1, 2, 4, 5}},
-})
-    -- EVIDENCE-OF: R-07548-13422 The ORDER BY clause on a DELETE statement
-    -- is used only to determine which rows fall within the LIMIT. The order
-    -- in which rows are deleted is arbitrary and is not influenced by the
-    -- ORDER BY clause.
-    --
-    --     In practice, rows are always deleted in rowid order.
-    --
-    test:do_delete_tests("e_delete-3.10", "-repair", [[
- rebuild_t1 
- catchsql { DROP TABLE t1log }
- execsql {
-   CREATE TABLE t1log(x INT );
-   CREATE TRIGGER tr1 AFTER DELETE ON t1 BEGIN
-     INSERT INTO t1log VALUES(old.a);
-   END;
- }
-]], "-query", [[
- SELECT x FROM t1log
-]], {
-   {1, "DELETE FROM t1 ORDER BY a DESC LIMIT 2", {4, 5}},
-   {2, "DELETE FROM t1 ORDER BY a DESC LIMIT -1", {1, 2, 3, 4, 5}},
-   {3, "DELETE FROM t1 ORDER BY a ASC LIMIT 2", {1, 2}},
-   {4, "DELETE FROM t1 ORDER BY a ASC LIMIT -1", {1, 2, 3, 4, 5}},
-})
 end
 
-
 test:finish_test()
-- 
2.14.3 (Apple Git-98)

[tarantool-patches] Re: [PATCH] sql: remove tests on ORDER BY/LIMIT + DELETE

[n.pettik]

> On 4 Feb 2019, at 18:34, Ivan Koptelov <ivan.koptelov@tarantool.org> wrote:
> 
> Removes obsolete tests on ORDER BY/LIMIT + DELETE. This
> functionality was not supported for some time, but a few tests
> were still existing.
> 
> Closes #2172
> ---
> Branch https://github.com/tarantool/tarantool/compare/sudobobo/gh-2172-rm-order-by-and-limit-from-delete
> Issue https://github.com/tarantool/tarantool/issues/2172
> 
> test/sql-tap/e_delete.test.lua | 188 -----------------------------------------
> 1 file changed, 188 deletions(-)
> 
> diff --git a/test/sql-tap/e_delete.test.lua b/test/sql-tap/e_delete.test.lua
> index 84a4e0a22..0a81eb3bd 100755
> --- a/test/sql-tap/e_delete.test.lua
> +++ b/test/sql-tap/e_delete.test.lua
> @@ -305,194 +305,6 @@ if (0 > 0) then
>          DELETE FROM t8 NOT INDEXED WHERE a=5;
>        END;]], {"NOT INDEXED"}},
>     })
> -    -- EVIDENCE-OF: R-64942-06615 The LIMIT and ORDER BY clauses (described
> -    -- below) are unsupported for DELETE statements within triggers.
> -    —

If I’m not mistaken, tests starting from e_delete-2.0 are disabled
(under if 0 > 0). So you can delete even more tests.

Also, check SQLITE_ENABLE_UPDATE_DELETE_LIMIT define:
I guess it is related to this patch. Seems that it is turned off, so I
assume we can delete it alongside with code under it.  

[tarantool-patches] Re: [PATCH] sql: remove tests on ORDER BY/LIMIT + DELETE

[Ivan Koptelov]

[-- Attachment #1: Type: text/html, Size: 2575 bytes --]

[tarantool-patches] Re: [PATCH] sql: remove tests on ORDER BY/LIMIT + DELETE

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]



> On 4 Feb 2019, at 22:11, Ivan Koptelov <ivan.koptelov@tarantool.org> wrote:
>>> On 4 Feb 2019, at 18:34, Ivan Koptelov <ivan.koptelov@tarantool.org> <mailto:ivan.koptelov@tarantool.org> wrote:
>>> 
>>> Removes obsolete tests on ORDER BY/LIMIT + DELETE. This
>>> functionality was not supported for some time, but a few tests
>>> were still existing.
>>> 
>>> Closes #2172
>>> ---
>>> Branch https://github.com/tarantool/tarantool/compare/sudobobo/gh-2172-rm-order-by-and-limit-from-delete <https://github.com/tarantool/tarantool/compare/sudobobo/gh-2172-rm-order-by-and-limit-from-delete>
>>> Issue https://github.com/tarantool/tarantool/issues/2172 <https://github.com/tarantool/tarantool/issues/2172>
>>> 
>>> test/sql-tap/e_delete.test.lua | 188 -----------------------------------------
>>> 1 file changed, 188 deletions(-)
>>> 
>>> diff --git a/test/sql-tap/e_delete.test.lua b/test/sql-tap/e_delete.test.lua
>>> index 84a4e0a22..0a81eb3bd 100755
>>> --- a/test/sql-tap/e_delete.test.lua
>>> +++ b/test/sql-tap/e_delete.test.lua
>>> @@ -305,194 +305,6 @@ if (0 > 0) then
>>>          DELETE FROM t8 NOT INDEXED WHERE a=5;
>>>        END;]], {"NOT INDEXED"}},
>>>     })
>>> -    -- EVIDENCE-OF: R-64942-06615 The LIMIT and ORDER BY clauses (described
>>> -    -- below) are unsupported for DELETE statements within triggers.
>>> -    —
>> If I’m not mistaken, tests starting from e_delete-2.0 are disabled
>> (under if 0 > 0). So you can delete even more tests.
> I thought we could enable this tests later?

Could you please examine those tests. If they are valid, simply uncomment them.
If not so - remove. Do it in a separate patch/commit pls.
>> Also, check SQLITE_ENABLE_UPDATE_DELETE_LIMIT define:
>> I guess it is related to this patch. Seems that it is turned off, so I
>> assume we can delete it alongside with code under it.  
> Yes, it turned off currently, but it may be turned on. This option also enables
> UPDATE + LIMIT/ORDER BY statements and all code under 'define' appplies
> to this functionality as well, so firstly I decided not to delete it. 

I really doubt that some day we will do it. LIMIT is non-ansi extension.
For example Oracle doesn’t support it at all. I guess this is a reason
why we got rid of delete+limit. Lets remove it as well.


[-- Attachment #2: Type: text/html, Size: 6987 bytes --]

[tarantool-patches] Re: [PATCH] sql: remove tests on ORDER BY/LIMIT + DELETE

[i.koptelov]

>> On 4 Feb 2019, at 22:11, Ivan Koptelov <ivan.koptelov@tarantool.org> wrote:
>>>> On 4 Feb 2019, at 18:34, Ivan Koptelov <ivan.koptelov@tarantool.org>
>>>>  wrote:
>>>> 
>>>> Removes obsolete tests on ORDER BY/LIMIT + DELETE. This
>>>> functionality was not supported for some time, but a few tests
>>>> were still existing.
>>>> 
>>>> Closes #2172
>>>> ---
>>>> Branch 
>>>> https://github.com/tarantool/tarantool/compare/sudobobo/gh-2172-rm-order-by-and-limit-from-delete
>>>> 
>>>> Issue 
>>>> https://github.com/tarantool/tarantool/issues/2172
>>>> 
>>>> 
>>>> test/sql-tap/e_delete.test.lua | 188 -----------------------------------------
>>>> 1 file changed, 188 deletions(-)
>>>> 
>>>> diff --git a/test/sql-tap/e_delete.test.lua b/test/sql-tap/e_delete.test.lua
>>>> index 84a4e0a22..0a81eb3bd 100755
>>>> --- a/test/sql-tap/e_delete.test.lua
>>>> +++ b/test/sql-tap/e_delete.test.lua
>>>> @@ -305,194 +305,6 @@ if (0 > 0) then
>>>>          DELETE FROM t8 NOT INDEXED WHERE a=5;
>>>>        END;]], {"NOT INDEXED"}},
>>>>     })
>>>> -    -- EVIDENCE-OF: R-64942-06615 The LIMIT and ORDER BY clauses (described
>>>> -    -- below) are unsupported for DELETE statements within triggers.
>>>> -    —
>>>> 
>>> If I’m not mistaken, tests starting from e_delete-2.0 are disabled
>>> (under if 0 > 0). So you can delete even more tests.
>>> 
>> I thought we could enable this tests later?
> 
> Could you please examine those tests. If they are valid, simply uncomment them.
> If not so - remove. Do it in a separate patch/commit pls.
They are not valid and test functionality we don’t have. I would send a separate small patch
with removing these tests.

>>> Also, check SQLITE_ENABLE_UPDATE_DELETE_LIMIT define:
>>> I guess it is related to this patch. Seems that it is turned off, so I
>>> assume we can delete it alongside with code under it.  
>>> 
>> Yes, it turned off currently, but it may be turned on. This option also enables
>> UPDATE + LIMIT/ORDER BY statements and all code under 'define' appplies
>> to this functionality as well, so firstly I decided not to delete it. 
> 
> I really doubt that some day we will do it. LIMIT is non-ansi extension.
> For example Oracle doesn’t support it at all. I guess this is a reason
> why we got rid of delete+limit. Lets remove it as well.
Done.

diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 0bcf41594..646e95ebe 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -727,7 +727,6 @@ where_opt(A) ::= WHERE expr(X).       {A = X.pExpr;}
 
 ////////////////////////// The UPDATE command ////////////////////////////////
 //
-%ifndef SQLITE_ENABLE_UPDATE_DELETE_LIMIT
 cmd ::= with(C) UPDATE orconf(R) fullname(X) indexed_opt(I) SET setlist(Y)
         where_opt(W).  {
   sqlite3WithPush(pParse, C, 1);
@@ -738,7 +737,6 @@ cmd ::= with(C) UPDATE orconf(R) fullname(X) indexed_opt(I) SET setlist(Y)
   pParse->initiateTTrans = true;
   sqlite3Update(pParse,X,Y,W,R);
 }
-%endif
 
 %type setlist {ExprList*}
 %destructor setlist {sql_expr_list_delete(pParse->db, $$);}
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 6462467bc..4c6baaac6 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -579,26 +579,6 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
 	}
 #endif
 	switch (pExpr->op) {
-
-#if defined(SQLITE_ENABLE_UPDATE_DELETE_LIMIT)
-		/* The special operator TK_ROW means use the rowid for the first
-		 * column in the FROM clause.  This is used by the LIMIT and ORDER BY
-		 * clause processing on UPDATE and DELETE statements.
-		 */
-	case TK_ROW:{
-			SrcList *pSrcList = pNC->pSrcList;
-			struct SrcList_item *pItem;
-			assert(pSrcList && pSrcList->nSrc == 1);
-			pItem = pSrcList->a;
-			pExpr->op = TK_COLUMN;
-			pExpr->pTab = pItem->pTab;
-			pExpr->iTable = pItem->iCursor;
-			pExpr->iColumn = -1;
-			pExpr->affinity = AFFINITY_INTEGER;
-			break;
-		}
-#endif				/* defined(SQLITE_ENABLE_UPDATE_DELETE_LIMIT) */
-
 		/* A lone identifier is the name of a column.
 		 */
 	case TK_ID:{

[tarantool-patches] Re: [PATCH] sql: remove tests on ORDER BY/LIMIT + DELETE

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 4374 bytes --]

LGTM.

> On 7 Feb 2019, at 20:45, i.koptelov <ivan.koptelov@tarantool.org> wrote:
> 
>>> 
>>> On 4 Feb 2019, at 22:11, Ivan Koptelov <ivan.koptelov@tarantool.org> wrote:
>>>>> On 4 Feb 2019, at 18:34, Ivan Koptelov <ivan.koptelov@tarantool.org>
>>>>> wrote:
>>>>> 
>>>>> Removes obsolete tests on ORDER BY/LIMIT + DELETE. This
>>>>> functionality was not supported for some time, but a few tests
>>>>> were still existing.
>>>>> 
>>>>> Closes #2172
>>>>> ---
>>>>> Branch 
>>>>> https://github.com/tarantool/tarantool/compare/sudobobo/gh-2172-rm-order-by-and-limit-from-delete
>>>>> 
>>>>> Issue 
>>>>> https://github.com/tarantool/tarantool/issues/2172
>>>>> 
>>>>> 
>>>>> test/sql-tap/e_delete.test.lua | 188 -----------------------------------------
>>>>> 1 file changed, 188 deletions(-)
>>>>> 
>>>>> diff --git a/test/sql-tap/e_delete.test.lua b/test/sql-tap/e_delete.test.lua
>>>>> index 84a4e0a22..0a81eb3bd 100755
>>>>> --- a/test/sql-tap/e_delete.test.lua
>>>>> +++ b/test/sql-tap/e_delete.test.lua
>>>>> @@ -305,194 +305,6 @@ if (0 > 0) then
>>>>>         DELETE FROM t8 NOT INDEXED WHERE a=5;
>>>>>       END;]], {"NOT INDEXED"}},
>>>>>    })
>>>>> -    -- EVIDENCE-OF: R-64942-06615 The LIMIT and ORDER BY clauses (described
>>>>> -    -- below) are unsupported for DELETE statements within triggers.
>>>>> -    —
>>>>> 
>>>> If I’m not mistaken, tests starting from e_delete-2.0 are disabled
>>>> (under if 0 > 0). So you can delete even more tests.
>>>> 
>>> I thought we could enable this tests later?
>> 
>> Could you please examine those tests. If they are valid, simply uncomment them.
>> If not so - remove. Do it in a separate patch/commit pls.
> They are not valid and test functionality we don’t have. I would send a separate small patch
> with removing these tests.
> 
>>>> Also, check SQLITE_ENABLE_UPDATE_DELETE_LIMIT define:
>>>> I guess it is related to this patch. Seems that it is turned off, so I
>>>> assume we can delete it alongside with code under it.  
>>>> 
>>> Yes, it turned off currently, but it may be turned on. This option also enables
>>> UPDATE + LIMIT/ORDER BY statements and all code under 'define' appplies
>>> to this functionality as well, so firstly I decided not to delete it. 
>> 
>> I really doubt that some day we will do it. LIMIT is non-ansi extension.
>> For example Oracle doesn’t support it at all. I guess this is a reason
>> why we got rid of delete+limit. Lets remove it as well.
> Done.
> 
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 0bcf41594..646e95ebe 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -727,7 +727,6 @@ where_opt(A) ::= WHERE expr(X).       {A = X.pExpr;}
> 
> ////////////////////////// The UPDATE command ////////////////////////////////
> //
> -%ifndef SQLITE_ENABLE_UPDATE_DELETE_LIMIT
> cmd ::= with(C) UPDATE orconf(R) fullname(X) indexed_opt(I) SET setlist(Y)
>         where_opt(W).  {
>   sqlite3WithPush(pParse, C, 1);
> @@ -738,7 +737,6 @@ cmd ::= with(C) UPDATE orconf(R) fullname(X) indexed_opt(I) SET setlist(Y)
>   pParse->initiateTTrans = true;
>   sqlite3Update(pParse,X,Y,W,R);
> }
> -%endif
> 
> %type setlist {ExprList*}
> %destructor setlist {sql_expr_list_delete(pParse->db, $$);}
> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index 6462467bc..4c6baaac6 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -579,26 +579,6 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
> 	}
> #endif
> 	switch (pExpr->op) {
> -
> -#if defined(SQLITE_ENABLE_UPDATE_DELETE_LIMIT)
> -		/* The special operator TK_ROW means use the rowid for the first
> -		 * column in the FROM clause.  This is used by the LIMIT and ORDER BY
> -		 * clause processing on UPDATE and DELETE statements.
> -		 */
> -	case TK_ROW:{
> -			SrcList *pSrcList = pNC->pSrcList;
> -			struct SrcList_item *pItem;
> -			assert(pSrcList && pSrcList->nSrc == 1);
> -			pItem = pSrcList->a;
> -			pExpr->op = TK_COLUMN;
> -			pExpr->pTab = pItem->pTab;
> -			pExpr->iTable = pItem->iCursor;
> -			pExpr->iColumn = -1;
> -			pExpr->affinity = AFFINITY_INTEGER;
> -			break;
> -		}
> -#endif				/* defined(SQLITE_ENABLE_UPDATE_DELETE_LIMIT) */
> -
> 		/* A lone identifier is the name of a column.
> 		 */
> 	case TK_ID:{


[-- Attachment #2: Type: text/html, Size: 70264 bytes --]

[tarantool-patches] Re: [PATCH] sql: remove tests on ORDER BY/LIMIT + DELETE

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 5013 bytes --]



> On 7 Feb 2019, at 21:54, n.pettik <korablev@tarantool.org> wrote:
> 
> LGTM.

Oh, my apologies, I forget that I've already removed this define and tests
connected with it:
https://github.com/tarantool/tarantool/commit/43ed060f9539ed44a232ca81ad890b1099872cd9

I’m really sorry, we should skip this patch.

>> On 7 Feb 2019, at 20:45, i.koptelov <ivan.koptelov@tarantool.org <mailto:ivan.koptelov@tarantool.org>> wrote:
>> 
>>>> 
>>>> On 4 Feb 2019, at 22:11, Ivan Koptelov <ivan.koptelov@tarantool.org <mailto:ivan.koptelov@tarantool.org>> wrote:
>>>>>> On 4 Feb 2019, at 18:34, Ivan Koptelov <ivan.koptelov@tarantool.org <mailto:ivan.koptelov@tarantool.org>>
>>>>>> wrote:
>>>>>> 
>>>>>> Removes obsolete tests on ORDER BY/LIMIT + DELETE. This
>>>>>> functionality was not supported for some time, but a few tests
>>>>>> were still existing.
>>>>>> 
>>>>>> Closes #2172
>>>>>> ---
>>>>>> Branch 
>>>>>> https://github.com/tarantool/tarantool/compare/sudobobo/gh-2172-rm-order-by-and-limit-from-delete <https://github.com/tarantool/tarantool/compare/sudobobo/gh-2172-rm-order-by-and-limit-from-delete>
>>>>>> 
>>>>>> Issue 
>>>>>> https://github.com/tarantool/tarantool/issues/2172
>>>>>> 
>>>>>> 
>>>>>> test/sql-tap/e_delete.test.lua | 188 -----------------------------------------
>>>>>> 1 file changed, 188 deletions(-)
>>>>>> 
>>>>>> diff --git a/test/sql-tap/e_delete.test.lua b/test/sql-tap/e_delete.test.lua
>>>>>> index 84a4e0a22..0a81eb3bd 100755
>>>>>> --- a/test/sql-tap/e_delete.test.lua
>>>>>> +++ b/test/sql-tap/e_delete.test.lua
>>>>>> @@ -305,194 +305,6 @@ if (0 > 0) then
>>>>>>         DELETE FROM t8 NOT INDEXED WHERE a=5;
>>>>>>       END;]], {"NOT INDEXED"}},
>>>>>>    })
>>>>>> -    -- EVIDENCE-OF: R-64942-06615 The LIMIT and ORDER BY clauses (described
>>>>>> -    -- below) are unsupported for DELETE statements within triggers.
>>>>>> -    —
>>>>>> 
>>>>> If I’m not mistaken, tests starting from e_delete-2.0 are disabled
>>>>> (under if 0 > 0). So you can delete even more tests.
>>>>> 
>>>> I thought we could enable this tests later?
>>> 
>>> Could you please examine those tests. If they are valid, simply uncomment them.
>>> If not so - remove. Do it in a separate patch/commit pls.
>> They are not valid and test functionality we don’t have. I would send a separate small patch
>> with removing these tests.
>> 
>>>>> Also, check SQLITE_ENABLE_UPDATE_DELETE_LIMIT define:
>>>>> I guess it is related to this patch. Seems that it is turned off, so I
>>>>> assume we can delete it alongside with code under it.  
>>>>> 
>>>> Yes, it turned off currently, but it may be turned on. This option also enables
>>>> UPDATE + LIMIT/ORDER BY statements and all code under 'define' appplies
>>>> to this functionality as well, so firstly I decided not to delete it. 
>>> 
>>> I really doubt that some day we will do it. LIMIT is non-ansi extension.
>>> For example Oracle doesn’t support it at all. I guess this is a reason
>>> why we got rid of delete+limit. Lets remove it as well.
>> Done.
>> 
>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>> index 0bcf41594..646e95ebe 100644
>> --- a/src/box/sql/parse.y
>> +++ b/src/box/sql/parse.y
>> @@ -727,7 +727,6 @@ where_opt(A) ::= WHERE expr(X).       {A = X.pExpr;}
>> 
>> ////////////////////////// The UPDATE command ////////////////////////////////
>> //
>> -%ifndef SQLITE_ENABLE_UPDATE_DELETE_LIMIT
>> cmd ::= with(C) UPDATE orconf(R) fullname(X) indexed_opt(I) SET setlist(Y)
>>         where_opt(W).  {
>>   sqlite3WithPush(pParse, C, 1);
>> @@ -738,7 +737,6 @@ cmd ::= with(C) UPDATE orconf(R) fullname(X) indexed_opt(I) SET setlist(Y)
>>   pParse->initiateTTrans = true;
>>   sqlite3Update(pParse,X,Y,W,R);
>> }
>> -%endif
>> 
>> %type setlist {ExprList*}
>> %destructor setlist {sql_expr_list_delete(pParse->db, $$);}
>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>> index 6462467bc..4c6baaac6 100644
>> --- a/src/box/sql/resolve.c
>> +++ b/src/box/sql/resolve.c
>> @@ -579,26 +579,6 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
>> 	}
>> #endif
>> 	switch (pExpr->op) {
>> -
>> -#if defined(SQLITE_ENABLE_UPDATE_DELETE_LIMIT)
>> -		/* The special operator TK_ROW means use the rowid for the first
>> -		 * column in the FROM clause.  This is used by the LIMIT and ORDER BY
>> -		 * clause processing on UPDATE and DELETE statements.
>> -		 */
>> -	case TK_ROW:{
>> -			SrcList *pSrcList = pNC->pSrcList;
>> -			struct SrcList_item *pItem;
>> -			assert(pSrcList && pSrcList->nSrc == 1);
>> -			pItem = pSrcList->a;
>> -			pExpr->op = TK_COLUMN;
>> -			pExpr->pTab = pItem->pTab;
>> -			pExpr->iTable = pItem->iCursor;
>> -			pExpr->iColumn = -1;
>> -			pExpr->affinity = AFFINITY_INTEGER;
>> -			break;
>> -		}
>> -#endif				/* defined(SQLITE_ENABLE_UPDATE_DELETE_LIMIT) */
>> -
>> 		/* A lone identifier is the name of a column.
>> 		 */
>> 	case TK_ID:{
> 


[-- Attachment #2: Type: text/html, Size: 71429 bytes --]

[tarantool-patches] Re: [PATCH] sql: remove tests on ORDER BY/LIMIT + DELETE

[Konstantin Osipov]

* n.pettik <korablev@tarantool.org> [19/02/04 18:54]:

Why ever delete tests? Why not let them (expectedly) fail?

 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH] sql: remove tests on ORDER BY/LIMIT + DELETE

[n.pettik]

> On 8 Feb 2019, at 20:38, Konstantin Osipov <kostja@tarantool.org> wrote:
> 
> * n.pettik <korablev@tarantool.org> [19/02/04 18:54]:
> 
> Why ever delete tests? Why not let them (expectedly) fail?
> 

Could you please clarify the purpose of such tests?

> 
> -- 
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> http://tarantool.io - www.twitter.com/kostja_osipov
> 

[tarantool-patches] Re: [PATCH] sql: remove tests on ORDER BY/LIMIT + DELETE

[Konstantin Osipov]

* n.pettik <korablev@tarantool.org> [19/02/09 08:47]:
> > Why ever delete tests? Why not let them (expectedly) fail?
> Could you please clarify the purpose of such tests?

In this particular case I don't entirely rule out the possibility
of these features re-appearing in our product.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH] sql: remove tests on ORDER BY/LIMIT + DELETE

[n.pettik]

> On 11 Feb 2019, at 16:09, Konstantin Osipov <kostja@tarantool.org> wrote:
> 
> * n.pettik <korablev@tarantool.org> [19/02/09 08:47]:
>>> Why ever delete tests? Why not let them (expectedly) fail?
>> Could you please clarify the purpose of such tests?
> 
> In this particular case I don't entirely rule out the possibility
> of these features re-appearing in our product.

Are you kidding? It was you who supported idea of removing
this feature:

‘’'
You can always use WHERE pk IN (subquery expr) - this is the standard alternative.
ORDER BY ... LIMIT is only useful if your optimizer is very weak, otherwise you can do it efficiently with a standard compliant SQL, in relational spirit.
‘''

But OK, even if we decided to enable this thing, it wouldn’t
be possible without implementing it from a scratch.
Almost all code related to ORDER BY + DELETE was
removed a long ago, only a few lines remained.
And even those lines haven’t been kept up to date
during numerous refactoring/reworking patches
(aka dead code). Hence, those tests are completely
useless and there is no chance that they will be turned on
again someday.