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 4C1CB21FD7 for ; Tue, 24 Apr 2018 08:51:53 -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 DCVwygWhWX8A for ; Tue, 24 Apr 2018 08:51:53 -0400 (EDT) Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (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 0A00522BA0 for ; Tue, 24 Apr 2018 08:51:52 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/4] sql: add average tuple size calculation References: <1483b8989b9065e4353a49fa236ea6acc8fbed93.1524515002.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <46e6a842-2a0b-90fc-7da0-2268eb6d05e3@tarantool.org> Date: Tue, 24 Apr 2018 15:51:50 +0300 MIME-Version: 1.0 In-Reply-To: <1483b8989b9065e4353a49fa236ea6acc8fbed93.1524515002.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Nikita Pettik , tarantool-patches@freelists.org See my 5 comments below, thanks. On 23/04/2018 23:29, Nikita Pettik wrote: > Average tuple size can be calculated by dividing space size by index > tuple count. Thus, there is no need to hold this value as separate > member. Thus, this patch removes from struct Index and struct Table such > statistic. Note that now there are no partial indexes, so all indexes > feature the same average tuple size. Also, commented unstable tests. > > Part of #3253 > --- > src/box/sql/analyze.c | 31 +++++++--------- > src/box/sql/build.c | 44 ----------------------- > src/box/sql/pragma.c | 20 +++++++++-- > src/box/sql/select.c | 26 +++++--------- > src/box/sql/sqliteInt.h | 24 +++++++++---- > src/box/sql/where.c | 41 +++++++++++---------- > test/sql-tap/analyze9.test.lua | 81 +++++++++++++++++++++--------------------- > 7 files changed, 118 insertions(+), 149 deletions(-) > > diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c > index 665bfbcb5..7c16a2154 100644 > --- a/src/box/sql/analyze.c > +++ b/src/box/sql/analyze.c > @@ -1196,6 +1196,18 @@ sqlite3Analyze(Parse * pParse, Token * pName) > sqlite3VdbeAddOp0(v, OP_Expire); > } > > +ssize_t > +sql_index_tuple_size(struct space *space, struct index *idx) > +{ > + assert(space != NULL); > + assert(idx != NULL); > + assert(idx->def->space_id == space->def->id); > + ssize_t tuple_count = idx->vtab->size(idx); > + ssize_t space_size = space->vtab->bsize(space); 1. Lets use wrappers: index_size() and space_bsize() - they are defined already. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 0df8a71d4..391b7e0a2 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -1588,20 +1588,19 @@ generateSortTail(Parse * pParse, /* Parsing context */ > * the SQLITE_ENABLE_COLUMN_METADATA compile-time option is used. > */ > #ifdef SQLITE_ENABLE_COLUMN_METADATA > -#define columnType(A,B,C,D,E,F) columnTypeImpl(A,B,D,E,F) > +#define columnType(A,B,C,D,E) columnTypeImpl(A,B,D,E) > #else /* if !defined(SQLITE_ENABLE_COLUMN_METADATA) */ > -#define columnType(A,B,C,D,E,F) columnTypeImpl(A,B,F) > +#define columnType(A,B,C,D,E) columnTypeImpl(A,B) > #endif > static enum field_type > -columnTypeImpl(NameContext * pNC, Expr * pExpr, > +columnTypeImpl(NameContext * pNC, Expr * pExpr > #ifdef SQLITE_ENABLE_COLUMN_METADATA > - const char **pzOrigTab, const char **pzOrigCol, > + , const char **pzOrigTab, const char **pzOrigCol, 2. As I can see, the third argument is always NULL. Lets remove it too. > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 59662cf14..8ca8e808f 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -1396,6 +1396,11 @@ struct BusyHandler { > */ > #define IsPowerOfTwo(X) (((X)&((X)-1))==0) > > +#ifdef ZERO_OR_DIV > +#undef ZERO_OR_DIV > +#endif > +#define DIV_OR_ZERO(NUM, DENOM) (((DENOM) != 0) ? ((NUM) / (DENOM)) : 0) 3. Divide by 0: *exists* Programmers:https://pm1.narvii.com/6585/b5b717574d0d6250181c18aadd89fbe0b3c7bf3a_hq.jpg Lets just inline it. And what is ZERO_OR_DIV? > diff --git a/src/box/sql/where.c b/src/box/sql/where.c > index 2a2630281..51b53c2df 100644 > --- a/src/box/sql/where.c > +++ b/src/box/sql/where.c > @@ -2545,15 +2537,31 @@ whereLoopAddBtreeIndex(WhereLoopBuilder * pBuilder, /* The WhereLoop factory */ > * seek only. Then, if this is a non-covering index, add the cost of > * visiting the rows in the main table. > */ > - rCostIdx = > - pNew->nOut + 1 + > - (15 * pProbe->szIdxRow) / pSrc->pTab->szTabRow; > + struct space *space = > + space_by_id(SQLITE_PAGENO_TO_SPACEID(pProbe->tnum)); > + assert(space != NULL); > + struct index *idx = > + space_index(space, > + SQLITE_PAGENO_TO_INDEXID(pProbe->tnum)); > + assert(idx != NULL); > + /* > + * FIXME: currently, the procedure below makes no > + * sense, since there are no partial indexes, so > + * all indexes in the space feature the same > + * average tuple size. 4. Do not forget about Vinyl. In it even with no partial indexes different ones can contain different tuple count, tuples of different size (due to specific of secondary indexes disk data structure). Now it does not support SQL, but will do. > diff --git a/test/sql-tap/analyze9.test.lua b/test/sql-tap/analyze9.test.lua > index 4ce575e90..3b3d52f67 100755 > --- a/test/sql-tap/analyze9.test.lua > +++ b/test/sql-tap/analyze9.test.lua > +-- These tests are commented until query planer will be stable. 5. What do you mean saying 'unstable'? The test is flaky or incorrect?