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 511D66EC41; Tue, 1 Jun 2021 17:03:07 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 511D66EC41 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1622556187; bh=mNNPtlojIOvxeJAuFSAlwWfk91ml33LQf6Wr5npWxA4=; 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=NVMKKrPk3IzdC/QGZMxE818pWX/VQ7rosKR8NkokrsOW0LvGG0aqdcv/YA9yO24oS aCU4+uW20Q5/2akm/PuN7PwiSzH3YCronng7ZxaapjaLKjw/TLOB+dcQyaJ/6kVIIN 3584841jjoGAdpOPB9V4q+BEtlnHxL/C6/IYIsq0= Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 50F766EC43 for ; Tue, 1 Jun 2021 17:02:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 50F766EC43 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1lo4yc-0004PV-K1; Tue, 01 Jun 2021 17:02:39 +0300 To: Timur Safin Cc: tarantool-patches@dev.tarantool.org References: Message-ID: Date: Tue, 1 Jun 2021 17:02:38 +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: 4F1203BC0FB41BD9D5B0DA836B685C546075B3437CB362FF47FF2EE3FBA69D6F182A05F538085040B7036158459328BBBC1A30D7643DE7CB32766077E2B79B481155B1DC08ED2247 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7CB9C0C663C74145DEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637498EFEA6A90945838638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8DCCD3682538A195DA9CB03D305E5D1B1117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD182CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B66F6A3E018CF4DC80089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A582AD490B5A5E081EFA40AD7CD03F659672C31ECF786FE681D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75FBC5FED0552DA851410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34EA882B598A209811408F3E322B46166914D527B84F8052CEFE4105FE656A35FBBFBE3A981643E3AF1D7E09C32AA3244C8240B32E0D3DAED0BC05B5073C15B93233C9DC155518937F729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojt1KCq4WZgK5qjvrJ5/9L6w== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822B51244DDD86E4549AF85499F756CC67683D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion 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" Thank you for the patch. See 18 comments below. 1. Title of commit-message is too long. Please try to fit it to 50 symbols. On Tue, May 25, 2021 at 12:01:00PM +0300, Timur Safin wrote: > We need to modify explicit casts table according to the RFC developed > previously in /doc/rfc/5910-consistent-sql-lua-types.md. This patch > introduces changes where BOOLEAN type involved, thus, for simplicity > sake, we mark unchanged cells in the table below as '.' > 2. I believe is it better to use RFC name instead of this relative link. 3. Do you really need whole table? I believe it is overkill to have it here. You already mentioned RFC and it is simpler to just have description of explicit cast related to BOOLEAN. > Since now on BOOLEAN will be only compatible with itself and STRINGs > (and recursively with SCALAR, which includes both those types). We > remove all other possible combinations which are defined now, these > cells marked with '-'. > > | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 | > +---+---+---+---+---+---+---+---+---+---+---+---+---+ > 0. any | | | | | | | | | | | | | | > 1. unsigned | | . | . | . | . | - | | . | | | | | . | > 2. string | | . | . | . | . | S | . | . | | | | | . | > 4. double | | . | . | . | . | - | | . | | | | | . | > 5. integer | | . | . | . | . | - | | . | | | | | . | > 6. boolean | | - | Y | - | - | Y | | | | | | | . | > 7. varbinary | | | . | | | - | . | | | | | | . | > 3. number | | . | . | . | . | - | | . | | | | | . | > 9. decimal | | | | | | | | | | | | | | > 10. uuid | | | | | | | | | | | | | | > 11. array | | | | | | | | | | | | | | > 12. map | | | | | | | | | | | | | | > 8. scalar | | . | . | . | . | S | . | . | | | | | . | > +---+---+---+---+---+---+---+---+---+---+---+---+---+ > 4. Again this strange order in table. > Current patch fixes BOOLEAN to other types entries, simultaneously > updating corresponding tests. > > Full check for current conversion table will be committed later as > separate comit. > 5. Do you need to mention this is commit-message for BOOLEAN? > Relates to #5910, #6009 > Part of #4407 6. Wrong issue number. Also, not sure that it is right to include "Relates to". You already have link to issue and RFC. > --- > src/box/sql/mem.c | 37 -------------------------------- > test/sql-tap/cse.test.lua | 12 +++++------ > test/sql-tap/e_select1.test.lua | 2 +- > test/sql-tap/in1.test.lua | 16 +++++++------- > test/sql-tap/misc3.test.lua | 2 +- > test/sql/boolean.result | 38 +++++++-------------------------- > test/sql/types.result | 14 ++++-------- > 7 files changed, 28 insertions(+), 93 deletions(-) > > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > index b6ff6397f..d40366754 100644 > --- a/src/box/sql/mem.c > +++ b/src/box/sql/mem.c > @@ -526,15 +526,6 @@ int_to_str0(struct Mem *mem) > return mem_copy_str0(mem, str); > } > > -static inline int > -int_to_bool(struct Mem *mem) > -{ > - mem->u.b = mem->u.i != 0; > - mem->flags = MEM_Bool; > - mem->field_type = FIELD_TYPE_BOOLEAN; > - return 0; > -} > - > static inline int > str_to_str0(struct Mem *mem) > { > @@ -717,24 +708,6 @@ double_to_str0(struct Mem *mem) > return 0; > } > > -static inline int > -double_to_bool(struct Mem *mem) > -{ > - mem->u.b = mem->u.r != 0.; > - mem->flags = MEM_Bool; > - mem->field_type = FIELD_TYPE_BOOLEAN; > - return 0; > -} > - > -static inline int > -bool_to_int(struct Mem *mem) > -{ > - mem->u.u = (uint64_t)mem->u.b; > - mem->flags = MEM_UInt; > - mem->field_type = FIELD_TYPE_UNSIGNED; > - return 0; > -} > - > static inline int > bool_to_str0(struct Mem *mem) > { > @@ -766,8 +739,6 @@ mem_to_int(struct Mem *mem) > return bytes_to_int(mem); > if ((mem->flags & MEM_Real) != 0) > return double_to_int(mem); > - if ((mem->flags & MEM_Bool) != 0) > - return bool_to_int(mem); > return -1; > } > > @@ -803,8 +774,6 @@ mem_to_number(struct Mem *mem) > assert((mem->flags & MEM_PURE_TYPE_MASK) != 0); > if ((mem->flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0) > return 0; > - if ((mem->flags & MEM_Bool) != 0) > - return bool_to_int(mem); > if ((mem->flags & (MEM_Str | MEM_Blob)) != 0) { > if (bytes_to_int(mem) == 0) > return 0; > @@ -879,8 +848,6 @@ mem_cast_explicit(struct Mem *mem, enum field_type type) > return bytes_to_uint(mem); > if ((mem->flags & MEM_Real) != 0) > return double_to_int(mem); > - if ((mem->flags & MEM_Bool) != 0) > - return bool_to_int(mem); > return -1; > case FIELD_TYPE_STRING: > return mem_to_str(mem); > @@ -891,12 +858,8 @@ mem_cast_explicit(struct Mem *mem, enum field_type type) > case FIELD_TYPE_BOOLEAN: > if ((mem->flags & MEM_Bool) != 0) > return 0; > - if ((mem->flags & (MEM_UInt | MEM_Int)) != 0) > - return int_to_bool(mem); > if ((mem->flags & MEM_Str) != 0) > return str_to_bool(mem); > - if ((mem->flags & MEM_Real) != 0) > - return double_to_bool(mem); > return -1; > case FIELD_TYPE_VARBINARY: > if ((mem->flags & MEM_Blob) != 0) > diff --git a/test/sql-tap/cse.test.lua b/test/sql-tap/cse.test.lua > index e09f955a0..84ff936e6 100755 > --- a/test/sql-tap/cse.test.lua > +++ b/test/sql-tap/cse.test.lua > @@ -31,7 +31,7 @@ test:do_test( > INSERT INTO t1 VALUES(2,21,22,23,24,25); > ]] > return test:execsql [[ > - SELECT b, -b, ~b, NOT CAST(b AS BOOLEAN), NOT NOT CAST(b AS BOOLEAN), b-b, b+b, b*b, b/b, b FROM t1 > + SELECT b, -b, ~b, NOT (b <> 0), NOT NOT (b <> 0), b-b, b+b, b*b, b/b, b FROM t1 7. Since you already change this line, add spaces before and after all operations with two operands. > ]] > end, { > -- > @@ -102,7 +102,7 @@ test:do_execsql_test( > test:do_execsql_test( > "cse-1.6.3", > [[ > - SELECT CASE WHEN CAST(b AS BOOLEAN) THEN d WHEN CAST(e AS BOOLEAN) THEN f ELSE 999 END, b, c, d FROM t1 > + SELECT CASE WHEN b<>0 THEN d WHEN e<>0 THEN f ELSE 999 END, b, c, d FROM t1 8. Please add spaces before and after '<>'. > ]], { > -- > 13, 11, 12, 13, 23, 21, 22, 23 > @@ -112,7 +112,7 @@ test:do_execsql_test( > test:do_execsql_test( > "cse-1.6.4", > [[ > - SELECT b, c, d, CASE WHEN CAST(b AS BOOLEAN) THEN d WHEN CAST(e AS BOOLEAN) THEN f ELSE 999 END FROM t1 > + SELECT b, c, d, CASE WHEN b<>0 THEN d WHEN e<>0 THEN f ELSE 999 END FROM t1 9. Same. > ]], { > -- > 11, 12, 13, 13, 21, 22, 23, 23 > @@ -122,7 +122,7 @@ test:do_execsql_test( > test:do_execsql_test( > "cse-1.6.5", > [[ > - SELECT b, c, d, CASE WHEN false THEN d WHEN CAST(e AS BOOLEAN) THEN f ELSE 999 END FROM t1 > + SELECT b, c, d, CASE WHEN false THEN d WHEN e<>0 THEN f ELSE 999 END FROM t1 10. Same. > ]], { > -- > 11, 12, 13, 15, 21, 22, 23, 25 > @@ -132,7 +132,7 @@ test:do_execsql_test( > test:do_execsql_test( > "cse-1.7", > [[ > - SELECT a, -a, ~a, NOT CAST(a AS BOOLEAN), NOT NOT CAST(a AS BOOLEAN), a-a, a+a, a*a, a/a, a FROM t1 > + SELECT a, -a, ~a, NOT (a <> 0), NOT NOT (a <> 0), a-a, a+a, a*a, a/a, a FROM t1 11. Same as in 7. > ]], { > -- > 1, -1 ,-2, false, true, 0, 2, 1, 1, 1, 2, -2, -3, false, true, 0, 4, 4, 1, 2 > @@ -152,7 +152,7 @@ test:do_execsql_test( > test:do_execsql_test( > "cse-1.9", > [[ > - SELECT NOT CAST(b AS BOOLEAN), ~b, NOT NOT CAST(b AS BOOLEAN), b FROM t1 > + SELECT NOT (b <> 0), ~b, NOT NOT (b <>0), b FROM t1 12. Please add space after '<>'. > ]], { > -- > false, -12, true, 11, false, -22, true, 21 > diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua > index ab0faa376..28ea1d82f 100755 > --- a/test/sql-tap/e_select1.test.lua > +++ b/test/sql-tap/e_select1.test.lua > @@ -910,7 +910,7 @@ test:do_select_tests( > > {"3", "SELECT sum(b+1) FROM z1 NATURAL LEFT JOIN z3", {-43.06}}, > {"4", "SELECT sum(b+2) FROM z1 NATURAL LEFT JOIN z3", {-38.06}}, > - {"5", "SELECT sum(CAST(b IS NOT NULL AS INTEGER)) FROM z1 NATURAL LEFT JOIN z3", {5}}, > + {"5", "SELECT sum(CASE WHEN b IS NOT NULL THEN 1 ELSE 0 END) FROM z1 NATURAL LEFT JOIN z3", {5}}, > }) > > -- EVIDENCE-OF: R-26684-40576 Each non-aggregate expression in the > diff --git a/test/sql-tap/in1.test.lua b/test/sql-tap/in1.test.lua > index 4b51da6e8..0fb059760 100755 > --- a/test/sql-tap/in1.test.lua > +++ b/test/sql-tap/in1.test.lua > @@ -97,13 +97,13 @@ test:do_execsql_test( > -- > }) > > -test:do_execsql_test( > +test:do_catchsql_test( 13. Do you really need to change type of the test? I believe you can do the same you did in the previous test. > "in-1.7", > [[ > SELECT a+ 100*CAST((a BETWEEN 1 and 3) AS INTEGER) FROM t1 ORDER BY b > ]], { > -- > - 101, 102, 103, 4, 5, 6, 7, 8, 9, 10 > + 1, "Type mismatch: can not convert TRUE to integer" > -- > }) > > @@ -154,13 +154,13 @@ test:do_execsql_test( > -- > }) > > -test:do_execsql_test( > +test:do_catchsql_test( 14. Same. > "in-2.5", > [[ > SELECT a+100*(CAST(b IN (8,16,24) AS INTEGER)) FROM t1 ORDER BY b > ]], { > -- > - 1, 2, 103, 104, 5, 6, 7, 8, 9, 10 > + 1, "Type mismatch: can not convert FALSE to integer" > -- > }) > > @@ -204,13 +204,13 @@ test:do_execsql_test( > -- > }) > > -test:do_execsql_test( > +test:do_catchsql_test( 15. Same. > "in-2.10", > [[ > SELECT a FROM t1 WHERE LEAST(0, CAST(b IN (a,30) AS INT)) <> 0 > ]], { > -- > - > + 1, "Type mismatch: can not convert FALSE to integer" > -- > }) > > @@ -250,13 +250,13 @@ test:do_execsql_test( > -- > }) > > -test:do_execsql_test( > +test:do_catchsql_test( 16. Same. > "in-3.3", > [[ > SELECT a + 100*(CAST(b IN (SELECT b FROM t1 WHERE a<5) AS INTEGER)) FROM t1 ORDER BY b > ]], { > -- > - 101, 102, 103, 104, 5, 6, 7, 8, 9, 10 > + 1, "Type mismatch: can not convert TRUE to integer" > -- > }) > > diff --git a/test/sql-tap/misc3.test.lua b/test/sql-tap/misc3.test.lua > index 313484b5d..c2dc67355 100755 > --- a/test/sql-tap/misc3.test.lua > +++ b/test/sql-tap/misc3.test.lua > @@ -510,7 +510,7 @@ test:do_execsql_test( > test:do_execsql_test( > "misc-8.2", > [[ > - SELECT count(*) FROM t3 WHERE 1+CAST((b IN ('abc','xyz')) AS INTEGER)==2 > + SELECT count(*) FROM t3 WHERE b IN ('abc','xyz') 17. Somehow this change looks a bit too much. What checks this test? > ]], { > -- > 2 > diff --git a/test/sql/boolean.result b/test/sql/boolean.result > index 177a39fb9..b268eb2fe 100644 > --- a/test/sql/boolean.result > +++ b/test/sql/boolean.result > @@ -502,23 +502,13 @@ INSERT INTO t3 VALUES (4, false) > -- Check CAST from BOOLEAN to the other types. > SELECT cast(true AS INTEGER), cast(false AS INTEGER); > | --- > - | - metadata: > - | - name: COLUMN_1 > - | type: integer > - | - name: COLUMN_2 > - | type: integer > - | rows: > - | - [1, 0] > + | - null > + | - 'Type mismatch: can not convert TRUE to integer' > | ... > SELECT cast(true AS NUMBER), cast(false AS NUMBER); > | --- > - | - metadata: > - | - name: COLUMN_1 > - | type: number > - | - name: COLUMN_2 > - | type: number > - | rows: > - | - [1, 0] > + | - null > + | - 'Type mismatch: can not convert TRUE to number' > | ... > -- gh-4462: ensure that text representation is uppercase. > SELECT cast(true AS TEXT), cast(false AS TEXT); > @@ -545,25 +535,13 @@ SELECT cast(true AS BOOLEAN), cast(false AS BOOLEAN); > -- Check CAST to BOOLEAN from the other types. > SELECT cast(100 AS BOOLEAN), cast(1 AS BOOLEAN), cast(0 AS BOOLEAN); > | --- > - | - metadata: > - | - name: COLUMN_1 > - | type: boolean > - | - name: COLUMN_2 > - | type: boolean > - | - name: COLUMN_3 > - | type: boolean > - | rows: > - | - [true, true, false] > + | - null > + | - 'Type mismatch: can not convert 100 to boolean' > | ... > SELECT cast(0.123 AS BOOLEAN), cast(0.0 AS BOOLEAN); > | --- > - | - metadata: > - | - name: COLUMN_1 > - | type: boolean > - | - name: COLUMN_2 > - | type: boolean > - | rows: > - | - [true, false] > + | - null > + | - 'Type mismatch: can not convert 0.123 to boolean' > | ... > SELECT cast('true' AS BOOLEAN), cast('false' AS BOOLEAN); > | --- > diff --git a/test/sql/types.result b/test/sql/types.result > index 687ca3b15..d59cbef7d 100644 > --- a/test/sql/types.result > +++ b/test/sql/types.result > @@ -1035,11 +1035,8 @@ box.execute("SELECT CAST(18446744073709551615 AS SCALAR);") > ... > box.execute("SELECT CAST(18446744073709551615 AS BOOLEAN);") > --- > -- metadata: > - - name: COLUMN_1 > - type: boolean > - rows: > - - [true] > +- null > +- 'Type mismatch: can not convert 18446744073709551615 to boolean' > ... > box.execute("SELECT CAST('18446744073709551615' AS INTEGER);") > --- > @@ -1105,11 +1102,8 @@ box.execute("SELECT CAST(-1.5 AS UNSIGNED);") > ... > box.execute("SELECT CAST(true AS UNSIGNED);") > --- > -- metadata: > - - name: COLUMN_1 > - type: unsigned > - rows: > - - [1] > +- null > +- 'Type mismatch: can not convert TRUE to unsigned' > ... > box.execute("SELECT CAST('123' AS UNSIGNED);") > --- > -- > 2.29.2 > 18. Please add tests for this patch.