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 461BC2BC35 for ; Thu, 27 Sep 2018 16:24:10 -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 V8tRtUspoiWS for ; Thu, 27 Sep 2018 16:24:10 -0400 (EDT) Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (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 D71EF2BC33 for ; Thu, 27 Sep 2018 16:24:09 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 0/6] Introduce strict typing for SQL References: From: Vladislav Shpilevoy Message-ID: <72fd7ea4-cb9d-4c36-3bad-8375a9fc1184@tarantool.org> Date: Thu, 27 Sep 2018 23:24:08 +0300 MIME-Version: 1.0 In-Reply-To: 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 Hi! I was asked by Kirill to review this patchset, so here my review turns up. See 3 comments below. 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). > 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. > > 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? > column strings as well as integers and floats. Hence, it is temporary > disabled. We should come up with automatical tool to rework them. > > * To the one who will review: > I still consider this patch-set as raw, but anyway we are going > to introduce 'static' typing, which would substitute these > patches and provide all type checks and conversion during > query compilation (not at VDBE runtime). So main purpose of this > patch is to ban typeless columns, refactor tests and ability to send > type as metadata via IProto. > > Georgy Kirichenko (5): > sql: split conflict action and affinity for Expr > sql: annotate SQL functions with return type > sql: pass true types of columns to Tarantool > sql: enforce implicit type conversions > sql: return result-set type via IProto > > Nikita Pettik (1): > sql: discard numeric conversion by unary plus > > extra/mkkeywordhash.c | 21 +- > src/box/execute.c | 11 +- > src/box/iproto_constants.h | 1 + > src/box/lua/lua_sql.c | 207 +++++++-------- > src/box/lua/net_box.c | 8 +- > src/box/sql.c | 95 +------ > src/box/sql/analyze.c | 9 +- > src/box/sql/build.c | 55 ++-- > src/box/sql/date.c | 16 +- > src/box/sql/expr.c | 181 ++++++++----- > src/box/sql/fkey.c | 2 +- > src/box/sql/func.c | 142 ++++++----- > src/box/sql/insert.c | 14 +- > src/box/sql/main.c | 7 +- > src/box/sql/parse.y | 104 ++++++-- > src/box/sql/select.c | 22 ++ > src/box/sql/sqliteInt.h | 63 +++-- > src/box/sql/treeview.c | 6 +- > src/box/sql/vdbe.c | 209 ++++++++++----- > src/box/sql/vdbe.h | 4 - > src/box/sql/vdbeInt.h | 10 +- > src/box/sql/vdbeapi.c | 19 +- > src/box/sql/vdbeaux.c | 2 +- > src/box/sql/vdbemem.c | 182 +++++++------ > src/box/sql/where.c | 4 +- > src/box/sql/wherecode.c | 11 +- > test/box/sql-update-with-nested-select.result | 2 +- > test/box/sql-update-with-nested-select.test.lua | 2 +- > test/sql-tap/affinity2.test.lua | 135 ---------- > test/sql-tap/aggnested.test.lua | 4 +- > test/sql-tap/alias.test.lua | 4 +- > test/sql-tap/alter.test.lua | 24 +- > test/sql-tap/alter2.test.lua | 16 +- > test/sql-tap/analyze1.test.lua | 12 +- > test/sql-tap/analyze3.test.lua | 31 ++- > test/sql-tap/analyze4.test.lua | 4 +- > test/sql-tap/analyze5.test.lua | 8 +- > test/sql-tap/analyze6.test.lua | 2 +- > test/sql-tap/analyze7.test.lua | 4 +- > test/sql-tap/analyze8.test.lua | 2 +- > test/sql-tap/analyze9.test.lua | 106 ++++---- > test/sql-tap/analyzeC.test.lua | 2 +- > test/sql-tap/analyzeD.test.lua | 4 +- > test/sql-tap/analyzeE.test.lua | 4 +- > test/sql-tap/analyzeF.test.lua | 8 +- > test/sql-tap/autoinc.test.lua | 28 +- > test/sql-tap/autoindex4.test.lua | 6 +- > test/sql-tap/badutf1.test.lua | 23 +- > test/sql-tap/boundary1.test.lua | 21 +- > test/sql-tap/boundary2.test.lua | 20 +- > test/sql-tap/boundary3.test.lua | 40 +-- > test/sql-tap/cast.test.lua | 43 +++- > test/sql-tap/check.test.lua | 26 +- > test/sql-tap/coalesce.test.lua | 2 +- > test/sql-tap/collation.test.lua | 9 +- > test/sql-tap/colname.test.lua | 22 +- > test/sql-tap/count.test.lua | 10 +- > test/sql-tap/cse.test.lua | 12 +- > test/sql-tap/date.test.lua | 4 +- > test/sql-tap/default.test.lua | 60 ++--- > test/sql-tap/delete4.test.lua | 16 +- > test/sql-tap/distinct.test.lua | 18 +- > test/sql-tap/distinctagg.test.lua | 2 +- > test/sql-tap/drop_all.test.lua | 2 +- > test/sql-tap/e_delete.test.lua | 28 +- > test/sql-tap/e_expr.test.lua | 137 ++++------ > test/sql-tap/e_select1.test.lua | 282 +++++++-------------- > test/sql-tap/eqp.test.lua | 22 +- > test/sql-tap/fkey1.test.lua | 34 +-- > test/sql-tap/fkey2.test.lua | 141 ++++++----- > test/sql-tap/fkey3.test.lua | 13 +- > test/sql-tap/fkey4.test.lua | 68 ++--- > test/sql-tap/func.test.lua | 142 +---------- > test/sql-tap/func5.test.lua | 8 +- > .../gh-2360-omit-truncate-in-transaction.test.lua | 2 +- > test/sql-tap/gh-2723-concurrency.test.lua | 6 +- > test/sql-tap/gh-2884-forbid-rowid-syntax.test.lua | 2 +- > test/sql-tap/gh-2996-indexed-by.test.lua | 2 +- > .../gh-3307-xfer-optimization-issue.test.lua | 28 +- > test/sql-tap/gh-3332-tuple-format-leak.test.lua | 2 +- > test/sql-tap/gh-3350-skip-scan.test.lua | 4 +- > test/sql-tap/gh2130-index-refer-table.test.lua | 4 +- > test/sql-tap/hexlit.test.lua | 27 +- > test/sql-tap/icu.test.lua | 4 +- > test/sql-tap/identifier-characters.test.lua | 6 +- > test/sql-tap/identifier_case.test.lua | 8 +- > test/sql-tap/in1.test.lua | 85 ++----- > test/sql-tap/in2.test.lua | 2 +- > test/sql-tap/in3.test.lua | 25 +- > test/sql-tap/in4.test.lua | 24 +- > test/sql-tap/in5.test.lua | 8 +- > test/sql-tap/index1.test.lua | 75 +++--- > test/sql-tap/index2.test.lua | 4 +- > test/sql-tap/index3.test.lua | 4 +- > test/sql-tap/index4.test.lua | 10 +- > test/sql-tap/index6.test.lua | 18 +- > test/sql-tap/index7.test.lua | 22 +- > test/sql-tap/insert1.test.lua | 24 +- > test/sql-tap/insert3.test.lua | 46 ++-- > test/sql-tap/intpkey.test.lua | 10 +- > test/sql-tap/join.test.lua | 50 ++-- > test/sql-tap/join2.test.lua | 6 +- > test/sql-tap/join3.test.lua | 2 +- > test/sql-tap/join5.test.lua | 20 +- > test/sql-tap/join6.test.lua | 18 +- > test/sql-tap/keyword1.test.lua | 6 +- > test/sql-tap/like2.test.lua | 6 +- > test/sql-tap/like3.test.lua | 74 +----- > test/sql-tap/limit.test.lua | 12 +- > test/sql-tap/lua_sql.test.lua | 22 +- > test/sql-tap/minmax2.test.lua | 6 +- > test/sql-tap/minmax3.test.lua | 8 +- > test/sql-tap/minmax4.test.lua | 4 +- > test/sql-tap/misc1.test.lua | 67 ++--- > test/sql-tap/misc3.test.lua | 18 +- > test/sql-tap/misc5.test.lua | 22 +- > test/sql-tap/null.test.lua | 10 +- > test/sql-tap/numcast.test.lua | 7 +- > test/sql-tap/offset1.test.lua | 4 +- > test/sql-tap/orderby1.test.lua | 12 +- > test/sql-tap/orderby2.test.lua | 18 +- > test/sql-tap/orderby4.test.lua | 8 +- > test/sql-tap/orderby5.test.lua | 40 +-- > test/sql-tap/orderby6.test.lua | 4 +- > test/sql-tap/orderby8.test.lua | 2 +- > test/sql-tap/orderby9.test.lua | 2 +- > test/sql-tap/printf2.test.lua | 10 +- > test/sql-tap/quote.test.lua | 12 +- > test/sql-tap/randexpr1.test.lua | 2 +- > test/sql-tap/resolver01.test.lua | 10 +- > test/sql-tap/select1.test.lua | 64 ++--- > test/sql-tap/select3.test.lua | 4 +- > test/sql-tap/select4.test.lua | 8 +- > test/sql-tap/select5.test.lua | 10 +- > test/sql-tap/select6.test.lua | 18 +- > test/sql-tap/select7.test.lua | 12 +- > test/sql-tap/select8.test.lua | 2 +- > test/sql-tap/select9.test.lua | 12 +- > test/sql-tap/selectA.test.lua | 48 ++-- > test/sql-tap/selectC.test.lua | 12 +- > test/sql-tap/selectE.test.lua | 10 +- > test/sql-tap/selectF.test.lua | 4 +- > test/sql-tap/selectG.test.lua | 2 +- > test/sql-tap/sort.test.lua | 124 +-------- > test/sql-tap/subquery.test.lua | 34 +-- > test/sql-tap/subquery2.test.lua | 10 +- > test/sql-tap/suite.ini | 8 +- > test/sql-tap/table.test.lua | 66 ++--- > test/sql-tap/tkt-02a8e81d44.test.lua | 2 +- > test/sql-tap/tkt-31338dca7e.test.lua | 22 +- > test/sql-tap/tkt-385a5b56b9.test.lua | 4 +- > test/sql-tap/tkt-38cb5df375.test.lua | 2 +- > test/sql-tap/tkt-3998683a16.test.lua | 2 +- > test/sql-tap/tkt-4a03edc4c8.test.lua | 2 +- > test/sql-tap/tkt-4c86b126f2.test.lua | 2 +- > test/sql-tap/tkt-4dd95f6943.test.lua | 10 +- > test/sql-tap/tkt-4ef7e3cfca.test.lua | 22 +- > test/sql-tap/tkt-54844eea3f.test.lua | 2 +- > test/sql-tap/tkt-7bbfb7d442.test.lua | 10 +- > test/sql-tap/tkt-80ba201079.test.lua | 10 +- > test/sql-tap/tkt-80e031a00f.test.lua | 18 +- > test/sql-tap/tkt-8c63ff0ec.test.lua | 4 +- > test/sql-tap/tkt-9a8b09f8e6.test.lua | 27 +- > test/sql-tap/tkt-a7b7803e.test.lua | 2 +- > test/sql-tap/tkt-a8a0d2996a.test.lua | 107 +------- > test/sql-tap/tkt-b1d3a2e531.test.lua | 16 +- > test/sql-tap/tkt-b351d95f9.test.lua | 4 +- > test/sql-tap/tkt-b75a9ca6b0.test.lua | 2 +- > test/sql-tap/tkt-ba7cbfaedc.test.lua | 2 +- > test/sql-tap/tkt-f973c7ac31.test.lua | 2 +- > test/sql-tap/tkt-fa7bf5ec.test.lua | 2 +- > test/sql-tap/tkt1443.test.lua | 14 +- > test/sql-tap/tkt1444.test.lua | 2 +- > test/sql-tap/tkt1449.test.lua | 78 +++--- > test/sql-tap/tkt1473.test.lua | 4 +- > test/sql-tap/tkt1501.test.lua | 2 +- > test/sql-tap/tkt1514.test.lua | 2 +- > test/sql-tap/tkt1537.test.lua | 4 +- > test/sql-tap/tkt2141.test.lua | 6 +- > test/sql-tap/tkt2192.test.lua | 2 +- > test/sql-tap/tkt2339.test.lua | 32 +-- > test/sql-tap/tkt2391.test.lua | 2 +- > test/sql-tap/tkt2640.test.lua | 6 +- > test/sql-tap/tkt2767.test.lua | 2 +- > test/sql-tap/tkt2822.test.lua | 29 +-- > test/sql-tap/tkt2832.test.lua | 6 +- > test/sql-tap/tkt2927.test.lua | 10 +- > test/sql-tap/tkt2942.test.lua | 10 +- > test/sql-tap/tkt3201.test.lua | 4 +- > test/sql-tap/tkt3298.test.lua | 4 +- > test/sql-tap/tkt3334.test.lua | 2 +- > test/sql-tap/tkt3346.test.lua | 8 +- > test/sql-tap/tkt3357.test.lua | 4 +- > test/sql-tap/tkt3424.test.lua | 2 +- > test/sql-tap/tkt3442.test.lua | 34 +-- > test/sql-tap/tkt3493.test.lua | 4 +- > test/sql-tap/tkt3508.test.lua | 2 +- > test/sql-tap/tkt3527.test.lua | 6 +- > test/sql-tap/tkt3541.test.lua | 2 +- > test/sql-tap/tkt3554.test.lua | 2 +- > test/sql-tap/tkt3581.test.lua | 4 +- > test/sql-tap/tkt3731.test.lua | 4 +- > test/sql-tap/tkt3773.test.lua | 4 +- > test/sql-tap/tkt3791.test.lua | 2 +- > test/sql-tap/tkt3879.test.lua | 6 +- > test/sql-tap/tkt3911.test.lua | 6 +- > test/sql-tap/tkt3935.test.lua | 4 +- > test/sql-tap/transitive1.test.lua | 28 +- > test/sql-tap/trigger1.test.lua | 12 +- > test/sql-tap/trigger2.test.lua | 48 ++-- > test/sql-tap/trigger4.test.lua | 6 +- > test/sql-tap/trigger7.test.lua | 4 +- > test/sql-tap/trigger8.test.lua | 4 +- > test/sql-tap/trigger9.test.lua | 14 +- > test/sql-tap/triggerA.test.lua | 18 +- > test/sql-tap/triggerB.test.lua | 20 +- > test/sql-tap/triggerC.test.lua | 240 +++--------------- > test/sql-tap/triggerD.test.lua | 20 +- > test/sql-tap/types.test.lua | 141 +---------- > test/sql-tap/types2.test.lua | 14 +- > test/sql-tap/unique.test.lua | 20 +- > test/sql-tap/update.test.lua | 18 +- > test/sql-tap/view.test.lua | 30 +-- > test/sql-tap/where2.test.lua | 14 +- > test/sql-tap/where3.test.lua | 54 ++-- > test/sql-tap/where4.test.lua | 30 +-- > test/sql-tap/where5.test.lua | 2 +- > test/sql-tap/where6.test.lua | 6 +- > test/sql-tap/where7.test.lua | 6 +- > test/sql-tap/whereA.test.lua | 28 +- > test/sql-tap/whereB.test.lua | 126 ++++----- > test/sql-tap/whereC.test.lua | 2 +- > test/sql-tap/whereD.test.lua | 16 +- > test/sql-tap/whereF.test.lua | 14 +- > test/sql-tap/whereG.test.lua | 20 +- > test/sql-tap/whereI.test.lua | 8 +- > test/sql-tap/whereK.test.lua | 2 +- > test/sql-tap/with1.test.lua | 22 +- > test/sql-tap/with2.test.lua | 18 +- > test/sql/check-clear-ephemeral.result | 2 +- > test/sql/check-clear-ephemeral.test.lua | 2 +- > test/sql/clear.result | 2 +- > test/sql/clear.test.lua | 2 +- > test/sql/collation.result | 2 +- > test/sql/collation.test.lua | 2 +- > test/sql/delete-multiple-idx.result | 2 +- > test/sql/delete-multiple-idx.test.lua | 2 +- > test/sql/delete.result | 4 +- > test/sql/delete.test.lua | 4 +- > test/sql/drop-index.result | 2 +- > test/sql/drop-index.test.lua | 2 +- > test/sql/drop-table.result | 2 +- > test/sql/drop-table.test.lua | 2 +- > test/sql/errinj.result | 5 +- > test/sql/errinj.test.lua | 4 +- > test/sql/foreign-keys.result | 9 +- > test/sql/foreign-keys.test.lua | 4 +- > test/sql/gh-2929-primary-key.result | 10 +- > test/sql/gh-2929-primary-key.test.lua | 10 +- > test/sql/gh-3199-no-mem-leaks.result | 14 +- > test/sql/gh-3199-no-mem-leaks.test.lua | 6 +- > test/sql/gh2141-delete-trigger-drop-table.result | 2 +- > test/sql/gh2141-delete-trigger-drop-table.test.lua | 2 +- > test/sql/gh2251-multiple-update.result | 4 +- > test/sql/gh2251-multiple-update.test.lua | 4 +- > .../gh2808-inline-unique-persistency-check.result | 2 +- > ...gh2808-inline-unique-persistency-check.test.lua | 2 +- > test/sql/insert-unique.result | 2 +- > test/sql/insert-unique.test.lua | 2 +- > test/sql/iproto.result | 55 +++- > test/sql/iproto.test.lua | 8 +- > test/sql/max-on-index.result | 2 +- > test/sql/max-on-index.test.lua | 2 +- > test/sql/misc.result | 2 +- > test/sql/misc.test.lua | 2 +- > test/sql/on-conflict.result | 17 +- > test/sql/on-conflict.test.lua | 4 + > test/sql/persistency.result | 14 +- > test/sql/persistency.test.lua | 6 +- > test/sql/select-null.result | 2 +- > test/sql/select-null.test.lua | 2 +- > test/sql/sql-statN-index-drop.result | 8 +- > test/sql/sql-statN-index-drop.test.lua | 8 +- > test/sql/transition.result | 16 +- > test/sql/transition.test.lua | 11 +- > test/sql/triggers.result | 14 +- > test/sql/triggers.test.lua | 14 +- > test/sql/update-with-nested-select.result | 2 +- > test/sql/update-with-nested-select.test.lua | 2 +- > test/sql/view.result | 2 +- > test/sql/view.test.lua | 2 +- > 291 files changed, 2790 insertions(+), 3515 deletions(-) > delete mode 100755 test/sql-tap/affinity2.test.lua >