From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D774927E32 for ; Mon, 29 Oct 2018 08:39:57 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id iSDJAovw3y3h for ; Mon, 29 Oct 2018 08:39:57 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 2F8B527C58 for ; Mon, 29 Oct 2018 08:39:57 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] sql: remove nKey from struct BtCursor References: <20181026145926.15345-1-korablev@tarantool.org> <64674871-e5f0-0cb2-540c-d95cfce47409@tarantool.org> <1AF05058-8C8F-4E46-A541-AEA551675E80@tarantool.org> From: Vladislav Shpilevoy Message-ID: <964ecb4f-495e-0256-9c6f-f74d77d3663c@tarantool.org> Date: Mon, 29 Oct 2018 15:39:54 +0300 MIME-Version: 1.0 In-Reply-To: <1AF05058-8C8F-4E46-A541-AEA551675E80@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" , tarantool-patches@freelists.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 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 = ;. Looks like >> it can be effectively forwarded to index_count(). > > 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 02 index id = 1, space ptr = space 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. >