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

[n.pettik]

> On 29 Oct 2018, at 13:27, Vladislav Shpilevoy <v.shpilevoy at 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.