[tarantool-patches] Re: [PATCH] sql: remove nKey from struct BtCursor

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the explanation. Then LGTM.

On 29/10/2018 15:25, n.pettik wrote:
> 
> 
>> On 29 Oct 2018, at 13:27, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>> Hi! Thanks for the patch!
>>
>> Consider my review fixes and the end of the email.
>>
>> Why are you sure that nKey is always 0? As I see
>> in the code, it is initialized in
>> tarantoolSqlite3MovetoUnpacked as
>> sqlite3VdbeMsgpackRecordLen.
>> How does COUNT work, when you specify WHERE? For example,
>> SELECT COUNT(*) WHERE key_column = <key_value>;. Looks like
>> it can be effectively forwarded to index_count(<key_value>).
> 
> OP_Count (ergo tarantoolSqlite3Count() function) is used only for queries
> without WHERE clause - grep is_simple_count() function. Otherwise, we
> rely on “generic" opcodes to perform any aggregate functions - OP_AggStep0,
> OP_AggStep and OP_AggFinal. In this case index is still used (however,
> may be not so effectively). Lets look at simple example:
> 
> CREATE TABLE t1 (id INT PRIMARY KEY, a INT);
> CREATE INDEX i1 ON t1 (a);
> INSERT INTO t1 VALUES (1,1), (2, -6), (3, 100), (6, -6);
> PRAGMA vdbe_debug = 1;
> SELECT COUNT(*) FROM t1 WHERE a = -6;
> 
> VDBE Trace:
>   101>    0 Init             0    1    0               00 Start at 1
> SQL-trace: select count(*) from t1 where a = -6
>   101>    1 Null             0    1    1               00 r[1..1]=NULL
> REG[1] =  NULL
>   101>    2 IteratorOpen     1    1    0 space<name=T1> 02 index id = 1, space ptr = space<name=T1> or reg[0]; I1
>   101>    3 Explain          0    0    0 SEARCH TABLE T1 USING COVERING INDEX I1 (A=?) 00
>   101>    4 Integer         -6    2    0               00 r[2]=-6
> REG[2] =  i:-6
>   101>    5 SeekGE           1    9    2 1             00 key=r[2]
> REG[2] =  i:-6
>   101>    7 AggStep0         0    0    1 count(0)      00 accum=r[1] step(r[0])
>   101>    8 Next             1    6    1               00
>   101>    6 IdxGT            1    9    2 1             00 key=r[2]
>   101>    7 AggStep          0    0    1 count(0)      00 accum=r[1] step(r[0])
>   101>    8 Next             1    6    1               00
>   101>    9 AggFinal         1    0    0 count(0)      00 accum=r[1] N=0
>   101>   10 Copy             1    3    0               00 r[3]=r[1]
> REG[3] =  i:2
>   101>   11 ResultRow        3    1    0               00 output=r[3]
> REG[3] =  i:2
>   101>   12 Halt             0    0    0               00
> 
> Here we open an iterator on index i1, seek for value -6 and iterate
> (alongside with counting) until there are elements with given value.
> 
> I guess, if we utilised index_count in form of:
> 
> index_count(idx, EQ, -6, nKey);
> 
> then it would be more efficient. On the other hand, such optimization
> is quite specific and available for queries with one filter condition under
> WHERE clause. Still it “has right to exist”.
> 
> *I must have put this explanation to the commit message, sorry for that.
>    I’ve extended commit message a bit with more explanation comments. *
> 
> If you don’t mind, I will open ticket concerning this optimisation.
> But at the current moment nKey doesn’t really make any sense.
> 
>> ===================================================
>>
>> diff --git a/src/box/sql.c b/src/box/sql.c
>> index 9d58b8fcf..9aca618ce 100644
>> --- a/src/box/sql.c
>> +++ b/src/box/sql.c
>> @@ -340,7 +340,7 @@ int tarantoolSqlite3EphemeralCount(struct BtCursor *pCur, i64 *pnEntry)
>> 	assert(pCur->curFlags & BTCF_TEphemCursor);
>>   	struct index *primary_index = space_index(pCur->space, 0 /* PK */);
>> -	*pnEntry = index_count(primary_index, pCur->iter_type, 0, 0);
>> +	*pnEntry = index_count(primary_index, pCur->iter_type, NULL, 0);
>> 	return SQLITE_OK;
>> }
>> @@ -348,7 +348,7 @@ int tarantoolSqlite3Count(BtCursor *pCur, i64 *pnEntry)
>> {
>> 	assert(pCur->curFlags & BTCF_TaCursor);
>> -	*pnEntry = index_count(pCur->index, pCur->iter_type, 0, 0);
>> +	*pnEntry = index_count(pCur->index, pCur->iter_type, NULL, 0);
>> 	return SQLITE_OK;
>> }
> 
> Ok, applied.
>