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 9D9F0229AF for ; Thu, 25 Jul 2019 18:10:25 -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 Q6A0N5EzJY65 for ; Thu, 25 Jul 2019 18:10:25 -0400 (EDT) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 5BC78227D1 for ; Thu, 25 Jul 2019 18:10:25 -0400 (EDT) Received: by smtp56.i.mail.ru with esmtpa (envelope-from ) id 1hqlwN-0006ym-Fj for tarantool-patches@freelists.org; Fri, 26 Jul 2019 01:10:23 +0300 Subject: [tarantool-patches] Re: [PATCH 5/5] sql: introduce VARBINARY column type References: From: Vladislav Shpilevoy Message-ID: Date: Fri, 26 Jul 2019 00:12:21 +0200 MIME-Version: 1.0 In-Reply-To: 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 Thanks for the patch! See 4 comments below. On 24/07/2019 13:42, Nikita Pettik wrote: > Current patch introduces new type available in SQL: > - VARBINARY now is reserved keyword; > - Allow to specify VARBINARY column and CAST type; > - All literals which start from 'x' are assumed to be of this type; > - There's no available implicit or explicit conversions between > VARBINARY and other types; > - Under the hood all values of VARBINARY type are stored as MP_BIN > msgpack format type. > > Closes #4206 > --- > extra/mkkeywordhash.c | 1 + > src/box/sql/expr.c | 2 +- > src/box/sql/func.c | 4 +- > src/box/sql/parse.y | 3 +- > src/box/sql/vdbe.c | 26 ++- > src/box/sql/vdbemem.c | 5 + > test/sql-tap/keyword1.test.lua | 3 +- > test/sql/gh-3888-values-blob-assert.result | 4 +- > test/sql/iproto.result | 4 +- > test/sql/misc.result | 2 +- > test/sql/types.result | 256 ++++++++++++++++++++++++++++- > test/sql/types.test.lua | 60 +++++++ > 12 files changed, 345 insertions(+), 25 deletions(-) > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 826232f99..d0f0cb4f5 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -2094,20 +2098,24 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */ > } > break; > } > - } else if ((flags1 | flags3) & MEM_Bool) { > + } else if (((flags1 | flags3) & MEM_Bool) != 0 || > + ((flags1 | flags3) & MEM_Blob) != 0) { 1. Nit: diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index d0f0cb4f5..ea7c9f25f 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -2098,8 +2098,7 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */ } break; } - } else if (((flags1 | flags3) & MEM_Bool) != 0 || - ((flags1 | flags3) & MEM_Blob) != 0) { + } else if (((flags1 | flags3) & (MEM_Bool | MEM_Blob)) != 0) { /* * If one of values is of type BOOLEAN or VARBINARY, * then the second one must be of the same type as 2. Please, add tests on varbinary values binding. 3. BLOB keyword is reserved, but also it is used in parse.y:980. Should not it be deleted from all the rules, and be just reserved? 4. Additionally (I bet, you knew I would ask), what are we going to do with all mentionings of blob in the code? We have word 'blob' mentioned 368 times in all the SQL sources, including comments, parts of names. In wherecode.c:644 we have a CREATE TABLE example, using BLOB - this one definitely should be fixed. What about other places? We could replace 'blob' -> 'bin','binary', 'vbin', ... . It should not take much time.