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 93BBB2D5B9 for ; Fri, 12 Oct 2018 07:18: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 xC0pBPr2u_5a for ; Fri, 12 Oct 2018 07:18:51 -0400 (EDT) Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 E24462D5B4 for ; Fri, 12 Oct 2018 07:18:50 -0400 (EDT) From: "n.pettik" Message-Id: <55FD67B5-3F6F-47A2-9850-494F2B957D64@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_7F4F8AA9-C7F2-4C3D-9E97-569B5CE8C491" Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 0/6] Introduce strict typing for SQL Date: Fri, 12 Oct 2018 14:18:40 +0300 In-Reply-To: <72fd7ea4-cb9d-4c36-3bad-8375a9fc1184@tarantool.org> References: <72fd7ea4-cb9d-4c36-3bad-8375a9fc1184@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: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy --Apple-Mail=_7F4F8AA9-C7F2-4C3D-9E97-569B5CE8C491 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > 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 = >> Issues: >> https://github.com/tarantool/tarantool/issues/2494 = >> https://github.com/tarantool/tarantool/issues/2620 = >> https://github.com/tarantool/tarantool/issues/2645 = >=20 > 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=E2=80=99m wrong. >> https://github.com/tarantool/tarantool/issues/3018 = >> https://github.com/tarantool/tarantool/issues/3104 = >> https://github.com/tarantool/tarantool/issues/3459 = >=20 > 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=E2=80=99ve put this mention to appropriate commit message to close = issue. >> This patch-set forces type specification for tables' columns. >> =46rom 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 >=20 > 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=E2=80=A6But 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=E2=80=99s = result... If you don=E2=80=99t insist, I will better open issue to resurrect this = test. Finally, I=E2=80=99ve rebased patch-set on fresh 2.0 (i.e. on top of DD = integration patches) and Travis is completely green. --Apple-Mail=_7F4F8AA9-C7F2-4C3D-9E97-569B5CE8C491 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

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=
Issues:
https://github.com/tarantool/tarantool/issues/2494
https://github.com/tarantool/tarantool/issues/2620
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=E2=80=99m wrong.

https://github.com/tarantool/tarantool/issues/3018
https://github.com/tarantool/tarantool/issues/3104
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=E2=80=99ve put this mention to appropriate = commit message to close issue.

This patch-set forces type specification for tables' = columns.
=46rom 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=E2=80=A6But 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=E2=80=99s result...
If you don=E2=80=99t insist, I will = better open issue to resurrect this test.

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


= --Apple-Mail=_7F4F8AA9-C7F2-4C3D-9E97-569B5CE8C491--