Tarantool development patches archive
 help / color / mirror / Atom feed
From: Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: "'Mergen Imeev'" <imeevma@tarantool.org>
Cc: <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables
Date: Thu, 3 Jun 2021 00:03:51 +0300	[thread overview]
Message-ID: <052d01d757f2$ca65cb90$5f3162b0$@tarantool.org> (raw)
In-Reply-To: <b19ec7eb-e2c3-298c-1d6c-73cfb6a54aca@tarantool.org>

: From: Mergen Imeev <imeevma@tarantool.org>
: Sent: Tuesday, June 1, 2021 5:03 PM
: To: Timur Safin <tsafin@tarantool.org>
: Cc: tarantool-patches@dev.tarantool.org
: Subject: Re: [PATCH 1/3] sql: fixes for boolean expressions in explicit
: converstion tables
: 
: 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.

Yup, will try

: 
: 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.

They are equivalent

: 
: 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.

This is local modification of a table, with all changes highlighted. 
As I said it elsewhere - picture worth thousands words. Verbal description
tends to be more verbose, and hard to skim. This simple table is very easy
to recognize the changed and unmodified.

: 
: > 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.

We will live with this order forever, due to RFC pictures. 

: 
: > 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?

Yup, I needed it at this version. Because I've updated all touched
tests in this commit, but larger test, with generated expressions
and fully covered matrix of all combination assumed to be committed
separately.

: 
: > 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.

Relates to creates links in GitHub for easier navigation between
related. And original discussion _is_ related to this whole process.

: 
: 
: > ---
: >  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.

Good point. Makes sense.

: 
: >          ]]
: >      end, {
: >          -- <cse-1.1>
: > @@ -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 '<>'.

Yup.

: 
: >      ]], {
: >          -- <cse-1.6.3>
: >          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.

Yup.

: 
: >      ]], {
: >          -- <cse-1.6.4>
: >          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.
: 
: >      ]], {
: >          -- <cse-1.6.5>
: >          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.
: 
: >      ]], {
: >          -- <cse-1.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 '<>'.
: 
: >      ]], {
: >          -- <cse-1.9>
: >          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(
: >          -- </in-1.6>
: >      })
: >
: > -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
: >      ]], {
: >          -- <in-1.7>
: > -        101, 102, 103, 4, 5, 6, 7, 8, 9, 10
: > +        1, "Type mismatch: can not convert TRUE to integer"
: >          -- </in-1.7>
: >      })
: >
: > @@ -154,13 +154,13 @@ test:do_execsql_test(
: >          -- </in-2.4>
: >      })
: >
: > -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
: >      ]], {
: >          -- <in-2.5>
: > -        1, 2, 103, 104, 5, 6, 7, 8, 9, 10
: > +        1, "Type mismatch: can not convert FALSE to integer"
: >          -- </in-2.5>
: >      })
: >
: > @@ -204,13 +204,13 @@ test:do_execsql_test(
: >          -- </in-2.9>
: >      })
: >
: > -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
: >      ]], {
: >          -- <in-2.10>
: > -
: > +        1, "Type mismatch: can not convert FALSE to integer"
: >          -- </in-2.10>
: >      })
: >
: > @@ -250,13 +250,13 @@ test:do_execsql_test(
: >          -- </in-3.2>
: >      })
: >
: > -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
: >      ]], {
: >          -- <in-3.3>
: > -        101, 102, 103, 104, 5, 6, 7, 8, 9, 10
: > +        1, "Type mismatch: can not convert TRUE to integer"
: >          -- </in-3.3>
: >      })
: >
: > 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?
: 
: >      ]], {
: >          -- <misc-8.2>
: >          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.


  reply	other threads:[~2021-06-02 21:03 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-25  9:00 [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables Timur Safin via Tarantool-patches
2021-05-25  9:01 ` [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables Timur Safin via Tarantool-patches
2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
2021-06-02 21:03     ` Timur Safin via Tarantool-patches [this message]
2021-06-02 21:10     ` Timur Safin via Tarantool-patches
2021-05-25  9:01 ` [Tarantool-patches] [PATCH 2/3] sql: enabled ANY as target for explicit conversions Timur Safin via Tarantool-patches
2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
2021-06-02 21:04     ` Timur Safin via Tarantool-patches
2021-05-25  9:01 ` [Tarantool-patches] [PATCH 3/3] sql: introduced explicit casts test e_casts.test.lua Timur Safin via Tarantool-patches
2021-06-01 14:02   ` Mergen Imeev via Tarantool-patches
2021-06-02 21:04     ` Timur Safin via Tarantool-patches
2021-06-01 14:02 ` [Tarantool-patches] [PATCH 0/3] sql: modify explicit conversion tables Mergen Imeev via Tarantool-patches
2021-06-02 21:04   ` Timur Safin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='052d01d757f2$ca65cb90$5f3162b0$@tarantool.org' \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=tsafin@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 1/3] sql: fixes for boolean expressions in explicit converstion tables' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox