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

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

[-- 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 --]