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 5D92421718 for ; Thu, 3 May 2018 06:27:09 -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 tB0va5qQVLjc for ; Thu, 3 May 2018 06:27:09 -0400 (EDT) Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 1AA02216FA for ; Thu, 3 May 2018 06:27:08 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v4 4/7] sql: start using collations and is_nullable from space_def References: <678424f1af156eb26d13ac58a2ed4befae0f98fd.1524939874.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 3 May 2018 13:21:06 +0300 MIME-Version: 1.0 In-Reply-To: <678424f1af156eb26d13ac58a2ed4befae0f98fd.1524939874.git.kshcherbatov@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: Kirill Shcherbatov , tarantool-patches@freelists.org Hello. See 5 comments below. On 28/04/2018 21:26, Kirill Shcherbatov wrote: > Part of #3272. > --- > src/box/sql.c | 36 +++++++++++++++++++++++++----------- > src/box/sql/alter.c | 11 ++++++++--- > src/box/sql/build.c | 20 +++++++++++++------- > src/box/sql/fkey.c | 7 ++----- > src/box/sql/select.c | 6 ++++-- > src/box/sql/sqliteInt.h | 7 ------- > src/box/sql/vdbeaux.c | 5 ++++- > 7 files changed, 56 insertions(+), 36 deletions(-) > > diff --git a/src/box/sql.c b/src/box/sql.c > index 2893d70..ef11eb9 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -1449,7 +1449,9 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf) > > for (i = 0; i < n; i++) { > const char *t; > - struct coll *coll = aCol[i].coll; > + struct coll *coll = > + coll_by_id(pTable->def->fields[i].coll_id); > + > struct field_def *field = &def->fields[i]; > const char *zToken = field->default_value; > int base_len = 4; > @@ -1461,19 +1463,25 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf) > p = enc->encode_str(p, "name", 4); > p = enc->encode_str(p, field->name, strlen(field->name)); > p = enc->encode_str(p, "type", 4); > + > + assert(def->fields[i].is_nullable == > + (def->fields[i].nullable_action == > + ON_CONFLICT_ACTION_NONE)); 1. AFAIR I asked you to wrap this check into a function. > diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c > index bedf602..f830a15 100644 > --- a/src/box/sql/alter.c > +++ b/src/box/sql/alter.c > @@ -307,9 +312,9 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc) > } > memcpy(pNew->aCol, pTab->aCol, sizeof(Column) * pNew->def->field_count); > for (i = 0; i < pNew->def->field_count; i++) { > - Column *pCol = &pNew->aCol[i]; > + /* FIXME: Column *pCol = &pNew->aCol[i]; */ 2. Why FIXME ? > diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c > index c15ad8c..8c015c9 100644 > --- a/src/box/sql/fkey.c > +++ b/src/box/sql/fkey.c > @@ -34,6 +34,7 @@ > * support to compiled SQL statements. > */ > #include > +#include "box/coll_cache.h" 3. Why? It is not used here. > @@ -535,11 +536,7 @@ exprTableRegister(Parse * pParse, /* Parsing and code generating context */ > pCol = &pTab->aCol[iCol]; > pExpr->iTable = regBase + iCol + 1; > pExpr->affinity = pCol->affinity; > - const char *coll_name; > - if (pCol->coll == NULL && pCol->coll != NULL) > - coll_name = pCol->coll->name; > - else > - coll_name = "binary"; > + const char *coll_name = "binary"; > pExpr = sqlite3ExprAddCollateString(pParse, pExpr, > coll_name); 4. Why you did not just inline "binary" string ? > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 03bfcf9..fa1de9b 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -1836,6 +1836,8 @@ sqlite3ColumnsFromExprList(Parse * pParse, /* Parsing context */ > pTable->def->fields = > region_alloc(region, nCol*sizeof(pTable->def->fields[0])); > memset(pTable->def->fields, 0, nCol*sizeof(pTable->def->fields[0])); > + for (int i = 0; i < nCol; i++) > + pTable->def->fields[i].is_nullable = true; 5. Why?