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

[n.pettik]

LGTM.

> On 7 Feb 2019, at 20:45, i.koptelov <ivan.koptelov at tarantool.org> wrote:
> 
>>> 
>>> On 4 Feb 2019, at 22:11, Ivan Koptelov <ivan.koptelov at tarantool.org> wrote:
>>>>> On 4 Feb 2019, at 18:34, Ivan Koptelov <ivan.koptelov at 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:{

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20190207/4d749ca3/attachment.html>