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 95A122A3C6 for ; Tue, 16 Apr 2019 10:12:51 -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 wijJp64HQ-Hq for ; Tue, 16 Apr 2019 10:12:51 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 4A08829F39 for ; Tue, 16 Apr 2019 10:12:51 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/9] sql: use msgpack types instead of custom ones References: <7d55169718d4bc193a41710da12cfd6a7d4edebc.1555252410.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <374916ec-2e49-643f-eccb-42f9d66e340e@tarantool.org> Date: Tue, 16 Apr 2019 17:12:48 +0300 MIME-Version: 1.0 In-Reply-To: <7d55169718d4bc193a41710da12cfd6a7d4edebc.1555252410.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8 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: tarantool-patches@freelists.org, Nikita Pettik Cc: kostja@tarantool.org Thanks for the patch! See 4 comments below. On 14/04/2019 18:04, Nikita Pettik wrote: > This patch provides straightforward refactoring replacing enum sql_type > with enum mp_type. Note that we use msgpack format instead of field_type > since it is more suitable when dealing with NULLs. > --- > src/box/bind.c | 37 ++++++------------ > src/box/execute.c | 12 +++--- > src/box/lua/execute.c | 7 +--- > src/box/lua/lua_sql.c | 12 +++--- > src/box/sql/date.c | 2 +- > src/box/sql/func.c | 100 ++++++++++++++++++++++++------------------------ > src/box/sql/legacy.c | 2 +- > src/box/sql/sqlInt.h | 16 ++------ > src/box/sql/vdbeapi.c | 66 ++++++++++---------------------- > src/box/sql/whereexpr.c | 2 +- > 10 files changed, 102 insertions(+), 154 deletions(-) > > diff --git a/src/box/bind.c b/src/box/bind.c > index 4b57e23c8..5aa79751a 100644 > --- a/src/box/bind.c > +++ b/src/box/bind.c > @@ -138,6 +121,7 @@ sql_bind_decode(struct sql_bind *bind, int i, const char **packet) > default: > unreachable(); > } > + bind->type = type; 1. struct sql_bind.type still is uint8_t and commented as 'SQL type of the value.'. Please, change its type to enum mp_type and say in the comment 'MessagePack type of the value.' > return 0; > } > > diff --git a/src/box/sql/date.c b/src/box/sql/date.c > index 5f5272ea3..ead0c01d0 100644 > --- a/src/box/sql/date.c > +++ b/src/box/sql/date.c > @@ -937,7 +937,7 @@ isDate(sql_context * context, int argc, sql_value ** argv, DateTime * p) > if (argc == 0) { > return setDateTimeToCurrent(context, p); > } > - if ((eType = sql_value_type(argv[0])) == SQL_FLOAT > + if ((eType = sql_value_type(argv[0])) == MP_DOUBLE 2. Firstly, the code is commented out. Secondly, sql_value_type does not exist anymore. Thirdly, eType is of type int, not enum mp_type, and can not be compared with MP_DOUBLE. Please, rename sql_value_mp_type to sql_value_type, because we do not have 'not mp' type anymore. Then this hunk will be ok except the third objection. > || eType == SQL_INTEGER) { > setRawDateNumber(p, sql_value_double(argv[0])); > } else { > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index 9adfeec67..3cdb119c8 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -87,10 +87,10 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv) > pColl = sqlGetFuncCollSeq(context); > assert(mask == -1 || mask == 0); > iBest = 0; > - if (sql_value_type(argv[0]) == SQL_NULL) > + if (sql_value_mp_type(argv[0]) == MP_NIL) 3. Please, move that check into a separate static inline function in a header file. It appeared to be used often. A possible name 'sql_value_is_null'. > return; > for (i = 1; i < argc; i++) { > - if (sql_value_type(argv[i]) == SQL_NULL) > + if (sql_value_mp_type(argv[i]) == MP_NIL) > return; > if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >= > 0) { > @@ -139,15 +139,15 @@ lengthFunc(sql_context * context, int argc, sql_value ** argv) > > assert(argc == 1); > UNUSED_PARAMETER(argc); > - switch (sql_value_type(argv[0])) { > - case SQL_BLOB: > - case SQL_INTEGER: > - case SQL_FLOAT:{ > + switch (sql_value_mp_type(argv[0])) { > + case MP_BIN: > + case MP_INT: > + case MP_DOUBLE:{ 4. Probably you could fix part of this: https://github.com/tarantool/tarantool/issues/4159 in scope of this commit, alongside. > sql_result_int(context, > sql_value_bytes(argv[0])); > break; > } > - case SQL_TEXT:{ > + case MP_STR:{ > const unsigned char *z = sql_value_text(argv[0]); > if (z == 0)