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 087F86EC58; Sun, 20 Jun 2021 21:52:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 087F86EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1624215173; bh=98i92ft7yP5QpwFj1qN2ejbx+4mTwV4QYs2gYnUI7rY=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=xUFNqL6Lt0gzUi11zs9OLZWdQMAx8bZUxeF5kgwynvsPkG+rIflfswXJlke8xwFoO kDM5Fqg4x0uzQAn9Rj2UbxweYK7ovSpFJeZNru8XotBilxkeZxkiaR9ypEG1GzCR2q Z+ZoV6tyG9qKcIxboA+yMQO9v9WVvvhd0toDme1c= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 35A126EC58 for ; Sun, 20 Jun 2021 21:52:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 35A126EC58 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lv2Ys-0006DE-4b; Sun, 20 Jun 2021 21:52:50 +0300 To: Timur Safin Cc: tarantool-patches@dev.tarantool.org, alexander.turenko@tarantool.org References: Message-ID: <3d3690b6-8f40-ffd4-568b-c0acbc2d5f92@tarantool.org> Date: Sun, 20 Jun 2021 21:52:49 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD91C2C07775F13263A70584CC5DB91BD99BA7C94740AA1B03F00894C459B0CD1B9F104992EBA8D3EEDCAAAE99438A083B7231B5704311BD6EEE5AAAE139CE2AE96 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE77BF46084C0059042EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063748424D8FCCA3295D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8BA6BB56B1B7A813CEFCD118633014AD3117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18C26CFBAC0749D213D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B60149FEE6F00FA24375ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A5BDEADC08C886D44E084FB2D274638B28B1DC9340D6357AC9D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34C264B329661203DADEAA2E486B46189B838C8840C9DF60F4C65E3B100278E18B9D2E4DB4DB916CCD1D7E09C32AA3244C9D4FC703E0126D3E5D54D3B5BFC781A55A1673A01BA68E40FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj8x+Gb+jwA+TfZG41Vhzgow== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822CBB7344FB5023BA09C42EADFD671DAB483D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B 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: Mergen Imeev via Tarantool-patches Reply-To: Mergen Imeev Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thank you for the patch-set. See 7 comments below. 0. Please include you answers to my previous comments right here. Same about all other letters. 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. 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. 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? > > Relates to #5910, #6009 > Part of #4470 > Fixes #6010 > > Explicit table > ============== > > 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? > > 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. Also, why you described BOOLEAN, but didn't describe ANY and UUID? > > 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. > - 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? 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. > 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. > > Implicit table > ============== > > 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. > > Relates to #5910, #6009 > Closes #4470 > > > Bonus - better error reporting in tap tests > =========================================== > > Also, during debugging of explicit/implicitit conversions test it was > discovered the harder way, that TAP infrastructure does not report correctly > origin source line of and error, but rather report :0 line as location. > This has been fixed. > > Closes #6134 > > > Timur Safin (3): > test: corrected reported error lines > sql: updated explicit conversion table > sql: updated implicit conversion table > > extra/mkkeywordhash.c | 3 +- > src/box/sql/mem.c | 188 +++-------- > src/box/sql/mem.h | 6 - > src/box/sql/parse.y | 9 +- > src/box/sql/util.c | 3 +- > src/box/sql/vdbe.c | 8 +- > src/box/sql/vdbeaux.c | 2 +- > src/lua/tap.lua | 2 +- > test/sql-tap/cse.test.lua | 12 +- > test/sql-tap/e_casts.test.lua | 474 +++++++++++++++++++++++++++ > test/sql-tap/e_select1.test.lua | 2 +- > test/sql-tap/in1.test.lua | 8 +- > test/sql-tap/in4.test.lua | 19 +- > test/sql-tap/misc3.test.lua | 2 +- > test/sql-tap/numcast.test.lua | 2 +- > test/sql-tap/tkt-9a8b09f8e6.test.lua | 40 +-- > test/sql/boolean.result | 38 +-- > test/sql/types.result | 132 ++++---- > 18 files changed, 654 insertions(+), 296 deletions(-) > create mode 100755 test/sql-tap/e_casts.test.lua > > -- > 2.29.2 >