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 0BD3921242 for ; Thu, 19 Apr 2018 08:58:58 -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 7KW-zcvfD5eN for ; Thu, 19 Apr 2018 08:58:57 -0400 (EDT) Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 A9F4121239 for ; Thu, 19 Apr 2018 08:58:57 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure. From: "n.pettik" In-Reply-To: <5eaf0628-8f05-eb95-1a4c-76b69e66cc00@tarantool.org> Date: Thu, 19 Apr 2018 15:58:53 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <1F5EB31E-147B-4373-8EFA-B1E158792997@tarantool.org> References: <60fd0daec8e35563602212ca8a3307ab49b14d15.1523896524.git.kshcherbatov@tarantool.org> <71db35b8-be0a-34be-07ab-09d63c4c39cd@tarantool.org> <07b96d39-dcd4-9900-defe-3f11b9b20537@tarantool.org> <951402e9-502b-d1c2-e9ef-9805bd56ac90@tarantool.org> <5eaf0628-8f05-eb95-1a4c-76b69e66cc00@tarantool.org> 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: Vladislav Shpilevoy , Kirill Shcherbatov Cc: tarantool-patches@freelists.org Hello. Several minor comments. Overall, seems OK to me. > +/** > + * Create and initialize a new SQL Table object. > + * @param pParse SQL Parser object. > + * @param zName Table to create name. > + * @retval NULL on memory allocation error, Parser state is > + * changed. > + * @retval not NULL on success. > + */ > +static Table * > +sql_table_new(Parse *pParse, char *zName) > +{ I would use Tarantool codestyle as well for args and vars inside function. It is not mandatory, but would look better. > +/** > + * Get field by id. Allocate memory if needed. > + * @param pParse SQL Parser object. > + * @param pTable SQL Table object. > + * @param id column identifier. > + * @retval not NULL on success. > + * @retval NULL on out of memory. > + */ > +static struct field_def * > +sql_field_retrieve(Parse *pParse, Table *pTable, uint32_t id) > +{ The same is here: you can use Tarantool codestyle for vars/args. Also, I strongly recommend you to add some explanation comments, in particular concerning allocating strategy. It doesn=E2=80=99t seem to be obvious (at least for independent = reviewer). > + sqlite3 *db =3D pParse->db; > + struct field_def *field; > + assert(pTable->def); Use explicit !=3D NULL check. > + assert(pTable->def->exact_field_count >=3D = (uint32_t)pTable->nCol); > + assert(id < (uint32_t)db->aLimit[SQLITE_LIMIT_COLUMN]); I have grepped through code, and it seems that this 'limit' is valid only under SQLITE_MAX_COLUMN macros. What about removing this macros (i.e. we will make this feature be = always enabled), or surrounding your code with #ifdef directives? > + > + if (id >=3D pTable->def->exact_field_count) { > + uint32_t nCol =3D pTable->def->exact_field_count; > + nCol =3D (nCol > 0) ? 2 * nCol : 1; > + field =3D sqlite3DbRealloc(db, pTable->def->fields, > + nCol * = sizeof(pTable->def->fields[0])); What about using simple malloc/realloc? It would allow you to get rid of pParse argument. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index b24d55b07..067f50a0a 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -1801,14 +1801,21 @@ xferOptimization(Parse * pParse, /* = Parser context */ > } > /* Default values for second and subsequent columns need = to match. */ > if (i > 0) { > - assert(pDestCol->pDflt =3D=3D 0 > - || pDestCol->pDflt->op =3D=3D TK_SPAN); > - assert(pSrcCol->pDflt =3D=3D 0 > - || pSrcCol->pDflt->op =3D=3D TK_SPAN); > - if ((pDestCol->pDflt =3D=3D 0) !=3D = (pSrcCol->pDflt =3D=3D 0) > - || (pDestCol->pDflt > - && strcmp(pDestCol->pDflt->u.zToken, > - pSrcCol->pDflt->u.zToken) !=3D = 0) > + uint32_t src_space_id =3D > + SQLITE_PAGENO_TO_SPACEID(pSrc->tnum); > + struct space *src_space =3D > + space_cache_find(src_space_id); space_cache_find() can return NULL pointer. I would check it on = nullability somehow. > + uint32_t dest_space_id =3D > + SQLITE_PAGENO_TO_SPACEID(pDest->tnum); > + struct space *dest_space =3D > + space_cache_find(dest_space_id); > + char *src_expr_str =3D > + src_space->def->fields[i].default_value; > + char *dest_expr_str =3D > + = dest_space->def->fields[i].default_value; > + if ((dest_expr_str =3D=3D NULL) !=3D = (src_expr_str =3D=3D NULL) > + || (dest_expr_str Codestyle nitpicking: put binary operator on previous line: =E2=80=A6 A && B=20