From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 773286EC40; Mon, 28 Jun 2021 02:29:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 773286EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1624836567; bh=DC6uXyLSERHfOjI6s/q68HCJB3d8oVCP4jfmhtKSImQ=; h=To:Cc:References:In-Reply-To:Date:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=PhG8FtqSCgbPmsUgPpWbHsQBmkiqeT13RDIRkfpOXjcAqeiWcUjFo9zXeoqbjbTm3 4/t/2QPQ+gk6QL6fgguM7a0clm6m5MWUq1gW4BQTF6seEkdqjLBCDcckHjWNgQeHUi tXnXVQSvUvXC5gslfDaPH5v/sK+5EhRY59ItXys8= Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id F0AA46EC40 for ; Mon, 28 Jun 2021 02:29:25 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org F0AA46EC40 Received: by smtp38.i.mail.ru with esmtpa (envelope-from ) id 1lxeDM-0007Lo-Tc; Mon, 28 Jun 2021 02:29:25 +0300 To: "'Mergen Imeev'" Cc: , References: <3d3690b6-8f40-ffd4-568b-c0acbc2d5f92@tarantool.org> In-Reply-To: <3d3690b6-8f40-ffd4-568b-c0acbc2d5f92@tarantool.org> Date: Mon, 28 Jun 2021 02:29:20 +0300 Message-ID: <151a01d76bac$41c0f2b0$c542d810$@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQH9YP+QCEY6e5VusJ0O6Ha7mUcGiQHcTesjqsqdANA= Content-Language: ru X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD954DFF1DC42D673FB96E19CC2B9345E2B1F8975EC27617E56182A05F538085040DB5630EA5A8AB24A62A3C9A97F9C5679A032B8B928A237EA65C6E9510409AC36 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE745C0EDBD94D46193EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063786D6A7D810B9582D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8731D22ECD7DEDBCA60E9FDFF28F30CA1117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC3A703B70628EAD7BA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EEC65AC60A1F0286FE8FBB52F5C7ECD1BBD8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3949686D59A6ADCEA6136E347CC761E07C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F79006373BC478629CBEC79DEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2368A440D3B0F6089093C9A16E5BC824A2A04A2ABAA09D25379311020FFC8D4AD70E3F9139E69115682D6B197855B54D4 X-C1DE0DAB: 0D63561A33F958A5C25C98880174B083E58468D24EB403325096C4A3886FD312D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA753C350047980234DB410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D75CEA0D7A80616E14AE04B75B3FB2996FA6D1A684CDF7A7E1CCA0426B01288202BF396A5A41A46C1D7E09C32AA3244C74D792D2C7F82B231D7A3FE0C02B00CBA8CE788DE6831205FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojBC0PCXY5SAUMWAqne4oHAA== X-Mailru-Sender: 6CA451E36783D721CBEA96CEA26D325D42CA454A01D165B344F02191DAFD973BB7CBEF92542CD7C82F97C478340294DCC77752E0C033A69E0F0C7111264B8915FF1320A92A5534336C18EFA0BB12DBB0 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Timur Safin via Tarantool-patches Reply-To: Timur Safin Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" : From: Mergen Imeev : Subject: Re: [PATCH v2 0/3] sql: modify explicit and implicit = conversion : tables :=20 : Hi! Thank you for the patch-set. See 7 comments below. :=20 : 0. Please include you answers to my previous comments right here. Same : about all other letters. ??? :=20 : On Fri, Jun 11, 2021 at 10:48:10AM +0300, Timur Safin wrote: : > Issues: : > https://github.com/tarantool/tarantool/issues/4470 : > https://github.com/tarantool/tarantool/issues/6010 : > Branch: https://github.com/tarantool/tarantool/tree/tsafin/gh-4470- : explicit-implicit-conversions-V2 : > : > This patchset containes of 2 major part and 1 bonus. Major parts : > are changes in explicit and implicit conversion tables, with all : > accompanying tests we have so far (and 1 extensive test introduced). : > The bonus part is for improvements in the testing system tap.lua : > which became apparent during debugging of this patches. : 1. Please divide this patch-set in two or three parts. I think the = best : way will : be to divide it to three parts: : i. Patch about tap.lua. : ii. Patch-set about explicit casts. : iii. Patch-set about implicit casts. Ok, I'll exclude patch with trivial tap fix (which has helped me=20 debug conversion tables test, but never mind). I'll think about a reasonable way to divide patches for implicit casts. Looks like there is easy way to extract part connected to separate ticket.=20 :=20 : Combining ii and iii is also acceptable, but I think the approaches to : ii and : iii should be completely different. I don't see any connection between = the : tap.lua patch and all the other patches in this patchset. :=20 : I think it is strange, that only part of the patch-set is reviewed by : me. Is it even possible for me to give LGTM to such patch-set? Yes, this is normal - you react to the patch which you know, and relay to others on other parts. =20 Let me explain it on some hypothetical example, not the current case. Imagine that there is big refactoring of some subsystem (say=20 introduction of cmake to luajit, with thousands of accompanying tests added), and say you are aware of cmake internals, and review it and send LGTM to only patches changing it, but are unaware of changes = in luajit testing, then you just not ack this part, and refer to others do their part. Everything is possible, and circumstances may require attention of various people, it's not problem if many people involved.=20 At the end of a day this calls "team work". :) :=20 : > : > Relates to #5910, #6009 : > Part of #4470 : > Fixes #6010 : > : > Explicit table : > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D : > : > As we discovered harder way, there is no need to introduce so much = changes : > to the current explicit conversion table, because it's mostly = compliant : > already: : > - most of changes we had to do was about `BOOLEAN` conversions, = which are : > stricter now than before; : > - there are addition of to `ANY` conversion, which we have decided = to make : > behaving similar to `SCALAR` conversions; : 2. As I previously said, I see no reason to include cast to field type : ANY in this patch-set. Also, if we follow your logic, why you didn't : added casts to array and map? Explicit casts to ANY allowed as part of RFC ratified last quarter, and = you have been part of team developed that decision. It's ok if we implement = something you disagree with, but respect the team decision. Arrays and maps not covered because they will be addressed later. :=20 : > : > Those changes could be visualized via this table: : > : > : > | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | = 8 | : > = +---+---+---+---+---+---+---+---+---+---+---+---+---+ : > 0. any | | | | | | | | | | | | | = | : > 1. unsigned | | . | . | . | . | - | | . | | | | | = Y | : > 2. string | | . | . | . | . | S | . | . | | | | | = Y | : > 4. double | | . | . | . | . | - | | . | | | | | = Y | : > 5. integer | | . | . | . | . | - | | . | | | | | = Y | : > 6. boolean | | - | Y | - | - | Y | | | | | | | = Y | : > 7. varbinary | | | . | | | - | . | | | | | | = Y | : > 3. number | | . | . | . | . | - | | . | | | | | = Y | : > 9. decimal | | | | | | | | | | | | | = | : > 10. uuid | | | | | | | | | | | | | = | : > 11. array | | | | | | | | | | | | | = | : > 12. map | | | | | | | | | | | | | = | : > 8. scalar | | . | . | . | . | S | . | . | | | | | = Y | : > = +---+---+---+---+---+---+---+---+---+---+---+---+---+ : 3. Since you do not want to change numbers here, I suggest to arrange = field : types according to their number.=20 I did not quite understand your sentence about numbers, could you please = clarify?=20 Also, I've already explained this strange order - this is exactly as = table described in RFC. I could reorder, but it would be harder to check = against original spec.=20 : Also, why you described BOOLEAN, but didn't : describe ANY and UUID? Good point - I will accumulate and highlight everything touched by the = patch, table was copied from original `boolean` specific commit. :=20 : > : > This table above represent all possible combinations of explicit = cast : > from type (row) to another type (column). Numbering of types might = look : > confusing (it's not sequential) but the order is what we see in the : > consistency specification RFC, and we should live with that. Values = are : > actual enum values from the code. Unfortunately, changing of order = is : > almost impossible because of massive rework for the spec it would = require. : > : > How to interpret this table, e.g.: : > - for the explicit cast from `BOOLEAN` (6) to `BOOLEAN` (6) we = should : > always allow cast (and make it noop), thus intersection is `Y` = (yes); : > : > - `STRING` (2) to `BOOLEAN` (6) may work sometimes (if string = represents : > TRUE or FALSE literals), but may fail otherwise, thus there is `S` : (sometimes); : > : 4. "Sometimes" still sounds quite not right here. Still not understand your complain here. For me it's quite ok. :=20 : > - We have prohibited majority of ther directions for `BOOLEAN` thus = there : are : > `-` (minus) in such cells, i.e. `BOOLEAN` (6) to `INTEGER` (5); : > : > - Untouched entries in the table marked with '.'. Assumption is - : > we already have correct conversion rule here activated well = before; : > : > - Worth to mention that empty cell means that this conversion : > direction is prohibited. : > : 5. Does it mean that cast from UUID to UUID is prohibited? Certainly it's not. Picture is incomplete, sorry for the confusion = created. I'll complete `boolean` only patch with all recent modifications.=20 [Now you probably see how visualization helps to narrow down problems?] : Also, I see no need : of '.' since you sometimes use 'Y' or '-' or ' ' instead of it. I : believe you : should describe table for all explicit cast conversions here and not : only for : BOOLEAN. Yes, will accumulate everything changed. And '.' means unmodified = 'Y'/'S' since prior state (pre-RFC implementation). :=20 :=20 : > For obvious reasons those chnaged conversion rules made us modify : > expected results for many tests - they are fixed. : > : > But to cover all possible conversion combinations (minus those, not = yet : > implemented in SQL types like DECIMAL, ARRAY and MAP) we have = created : > special test which check _all combinations of CASTs()_ and verifies : > results against table rules defined in the RFC. : > : > There is e_casts.test.lua created for that purpose. : 6. I still does not understand where this name came from. I believe = you said : something about renaming it. Yep, will do. :=20 : > : > Implicit table : > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D : > : > Similarly to explicit conversion rules we have update implicit = conversion : > rules, and this is 2nd part of patchset. : > : > We will not draw changed values of implicit conversion table (please = see : > it's final state in the consistemcy types RFC), but verbally we have : > modified following directions of conversions: : > - string to/from double : > - double to/from integer : > - varbinary to/from string : > : > In addition to modifciation of all relevant test results, we have = also : > extended e_casts.test.lua introduced above for checking of all = possible : > directions of implicit conversions. : > : > Approach to check it though is different than for explicit cast, we = do : > not use simple expression with implicit cast activated (like we = might be : > using `CAST()` for explicit casts), but rather we insert value of = original : > type into columns of expected target type. : 7. Implicit casts includes a lot more than just an insert, I believe. Yes, according to SQL-2003 standard "Implicit type conversion can occur = in expressions, fetch operations, single row select operations, inserts, = deletes, and updates." Insert looks the simplest way possible in the generated code to check = cast=20 against the expected result. I plan to extend test later, while adding = more casts for future types. [At the moment there are no concise and clear type promotion rules when = implicit casts done in expressions. There is some hope that currently ongoing = discussion might provide such promotion rules as discussion side-effect, then we = could=20 add those rules to the test. Not ready for such today. Yet] Regards, Timur