[tarantool-patches] Re: [PATCH 0/6] Introduce strict typing for SQL

[n.pettik]

> Hi! I was asked by Kirill to review this patchset, so here my
> review turns up. See 3 comments below.

Many thanks for provided review, I really appreciate it.

> On 17/09/2018 23:32, Nikita Pettik wrote:
>> Branch: https://github.com/tarantool/tarantool/tree/np/sql-static-types <https://github.com/tarantool/tarantool/tree/np/sql-static-types>
>> Issues:
>> https://github.com/tarantool/tarantool/issues/2494 <https://github.com/tarantool/tarantool/issues/2494>
>> https://github.com/tarantool/tarantool/issues/2620 <https://github.com/tarantool/tarantool/issues/2620>
>> https://github.com/tarantool/tarantool/issues/2645 <https://github.com/tarantool/tarantool/issues/2645>
> 
> 1. Why 2645? I do not see where do you check types
> in the parser. Also, Kirill said that it is relocated
> to 2.1.1/2.2.0 (I do not remember exact milestone).

I thought this ticket not about real static typing (i.e. implementing
all type checks and conversions during query compilation), but
concerning simple banning of things like:

CREATE TABLE t1 (id NOT_REALLY_INT PRIMARY KEY);

In other words, patch parser to accept only allowed by ANSI SQL types.
Correct me if I’m wrong.

>> https://github.com/tarantool/tarantool/issues/3018 <https://github.com/tarantool/tarantool/issues/3018>
>> https://github.com/tarantool/tarantool/issues/3104 <https://github.com/tarantool/tarantool/issues/3104>
>> https://github.com/tarantool/tarantool/issues/3459 <https://github.com/tarantool/tarantool/issues/3459>
> 
> 2. Is 3459 fixed? I do not see its mention in the
> commits.

Yep, it is automatically fixed within  #3104 i.e. when real type
hits space format, index type is displayed correctly.
Also, I put comment concerning this to the issue description.
I’ve put this mention to appropriate commit message to close issue.

>> This patch-set forces type specification for tables' columns.
>> From now, our grammar doesn't support typeless columns.
>> Allowed types are:
>> FLOAT, REAL, DOUBLE, NUMERIC, DECIMAL, INTEGER, TEXT, CHAR, VARCHAR,
>> BLOB, TIME, DATE, DATETIME.
>> Despite diversity of types, they are translated only to four types
>> from Tarantool's core: NUMBER, STRING, INT, SCALAR. Moreover,
>> restrictions such as length of CHAR type or precision of numeric type
>> are not involved now. Internally, AFFINITY is still used.
>> Fourth patch attempts at implementing implicit types conversions
>> to make it look closer to other DBs.
>> This patch-set refactors almost all tests in sql-tap/ suite.
>> Since number of tests is quite LARGE, some of them may contain
>> mistakes or strange behaviour. Initial plan was to automatically
>> spread INT type among all tables and fix failing tests. It turns out,
>> that not all tests can be converted: for instance, selectA.test.lua
>> originally almost in all subtests (more than 10k) inserts to one
> 
> 3. selectA.test.lua consists of 2536 lines. Why 10k? What is a
> problem to convert it manually?


Ok, mb I meant 10k in general…But anyway, selectA is unlikely
to be fixed, it is rather about completely rewrite this test.
And to make it fair, we should manually check each select’s result...
If you don’t insist, I will better open issue to resurrect this test.

Finally, I’ve rebased patch-set on fresh 2.0 (i.e. on top of DD integration
patches) and Travis is completely green.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20181012/97d74bee/attachment.html>