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 A4B532BC60 for ; Thu, 27 Sep 2018 16:24:02 -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 nDMzQrCuseax for ; Thu, 27 Sep 2018 16:24:02 -0400 (EDT) Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (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 2A8BC2BC33 for ; Thu, 27 Sep 2018 16:24:02 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/6] sql: pass true types of columns to Tarantool References: <9a94105515bd4f9148f302b24230d0918cfccdf9.1537216078.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 27 Sep 2018 23:23:57 +0300 MIME-Version: 1.0 In-Reply-To: <9a94105515bd4f9148f302b24230d0918cfccdf9.1537216078.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: tarantool-patches@freelists.org, Nikita Pettik Cc: Georgy Kirichenko See 3 comments below. On 17/09/2018 23:32, Nikita Pettik wrote: > From: Georgy Kirichenko > > As a main part of introducing strict typing in SQL it is required to > prohibit typeless columns in parser's grammar. Originally, SQLite simply > assigns typeless columns to BLOB affinity. Moreover, due to historical > reasons, all columns were stored with type in Tarantool core > (except for when it comes to primary key). Column type should > be defined on table creation. Allowed data types are: , , > , , , , , , , > and . However, still any declared data type is > converted to one of , , or affinities. > While affinity reaches space format, it is (again) converted to > Tarantool's field type. To be more precise, table of conversions: > > +----------+----------+------------+ > | SQL TYPE | AFFINITY | FIELD TYPE | > +----------+----------+------------+ > | FLOAT | REAL | NUMBER | > | REAL | REAL | NUMBER | > | DOUBLE | REAL | NUMBER | > | NUMERIC | REAL | NUMBER | > | DECIMAL | REAL | NUMBER | > | INTEGER | INTEGER | INTEGER | > | TEXT | TEXT | STRING | > | VARCHAR | TEXT | STRING | > | CHAR | TEXT | STRING | > | BLOB | BLOB | SCALAR | > | DATETIME | REAL | NUMBER | > | DATE | REAL | NUMBER | > | TIME | REAL | NUMBER | > +----------+----------+------------+ > > and types should be specified with length > (e.g. name VARCHAR(10)), but this length currently is not used when > types are processed. Only purpose is to support ANSI syntax. > The same for and - it is allowed to specify scale > and precision, but they don't affect the way they are stored in memory. > > Note that patch is not self-sufficient: a lot of tests still fail due to > wrong types conversions. Fix for that comes as next patch. > > Closes #3018 > Closes #3104 > Closes #2494 > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index b57e3811d..cde6bf996 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -99,6 +99,22 @@ sqlite3ExprAffinity(Expr * pExpr) > return sqlite3ExprAffinity(pExpr->pLeft->x.pSelect->pEList-> > a[pExpr->iColumn].pExpr); > } > + if (op == TK_PLUS) { > + assert(pExpr->pRight != NULL && pExpr->pLeft != NULL); > + enum affinity_type lhs_aff = sqlite3ExprAffinity(pExpr->pLeft); > + enum affinity_type rhs_aff = sqlite3ExprAffinity(pExpr->pRight); > + return sql_affinity_result(rhs_aff, lhs_aff); > + } > + if (op == TK_COLUMN) { > + assert(pExpr->space_def != NULL); > + const char *col_name = pExpr->u.zToken; > + size_t name_len = strlen(col_name); > + uint32_t field_no; > + tuple_fieldno_by_name(pExpr->space_def->dict, col_name, name_len, > + field_name_hash(col_name, name_len), > + &field_no); 1. Why are you sure that tuple_fieldno_by_name can not return an error? 2. This code is unreachable - in the same function on line 93 TK_COLUMN is processed already. What is curious - I removed this hunk entirely and next commit's tests passed. So op == TK_PLUS check here is not needed as well, is it? I guess, it is already processed in another place. > + return pExpr->space_def->fields[field_no].affinity; > + } > return pExpr->affinity; > } > > diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result > index 63fe48e79..290aa4162 100644 > --- a/test/sql/on-conflict.result > +++ b/test/sql/on-conflict.result > @@ -1,9 +1,17 @@ > test_run = require('test_run').new() > --- > ... > +--- > +... > +--- > +... 3. wtf ??? > engine = test_run:get_cfg('engine') > --- > ... > +--- > +... > +--- > +...