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

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

[Nikita Pettik]

nKey member (indicating number of parts in key) now is used only for
COUNT routine. On the other hand, it is always equal to 0 (as well as
key is really NULL). Hence, nothing prevents us from removing this
field at all.
---
https://github.com/tarantool/tarantool/tree/np/remove-nkey-from-cursor

 src/box/sql.c        | 7 ++-----
 src/box/sql/cursor.h | 1 -
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index c7b87e57a..9d58b8fcf 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -340,8 +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, pCur->key,
-			       pCur->nKey);
+	*pnEntry = index_count(primary_index, pCur->iter_type, 0, 0);
 	return SQLITE_OK;
 }
 
@@ -349,8 +348,7 @@ int tarantoolSqlite3Count(BtCursor *pCur, i64 *pnEntry)
 {
 	assert(pCur->curFlags & BTCF_TaCursor);
 
-	*pnEntry = index_count(pCur->index, pCur->iter_type, pCur->key,
-			       pCur->nKey);
+	*pnEntry = index_count(pCur->index, pCur->iter_type, 0, 0);
 	return SQLITE_OK;
 }
 
@@ -1031,7 +1029,6 @@ key_alloc(BtCursor *cur, size_t key_size)
 		}
 		cur->key = new_key;
 	}
-	cur->nKey = key_size;
 	return 0;
 }
 
diff --git a/src/box/sql/cursor.h b/src/box/sql/cursor.h
index da711a7de..d40ca87c8 100644
--- a/src/box/sql/cursor.h
+++ b/src/box/sql/cursor.h
@@ -41,7 +41,6 @@ typedef struct BtCursor BtCursor;
  * for ordinary space, or to TEphemCursor for ephemeral space.
  */
 struct BtCursor {
-	i64 nKey;		/* Size of pKey, or last integer key */
 	u8 curFlags;		/* zero or more BTCF_* flags defined below */
 	u8 eState;		/* One of the CURSOR_XXX constants (see below) */
 	u8 hints;		/* As configured by CursorSetHints() */
-- 
2.15.1

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

[Vladislav Shpilevoy]

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>).

===================================================

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;
  }

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

[n.pettik]

> 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.

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

[Vladislav Shpilevoy]

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.
> 

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

[Kirill Yukhin]

Hello,
On 26 Oct 17:59, Nikita Pettik wrote:
> nKey member (indicating number of parts in key) now is used only for
> COUNT routine. On the other hand, it is always equal to 0 (as well as
> key is really NULL). Hence, nothing prevents us from removing this
> field at all.
> ---
> https://github.com/tarantool/tarantool/tree/np/remove-nkey-from-cursor
I've pushed your patch into 2.1 branch.

--
Regards, Kirill Yukhin