Tarantool development patches archive
 help / color / mirror / Atom feed
From: Mergen Imeev <imeevma@tarantool.org>
To: Nikita Pettik <korablev@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER
Date: Fri, 10 Apr 2020 13:43:52 +0300	[thread overview]
Message-ID: <20200410104352.GA20241@tarantool.org> (raw)
In-Reply-To: <20200410103945.GA20019@tarantool.org>

Sorry, forgot to answer one question.

On Fri, Apr 10, 2020 at 01:39:45PM +0300, Mergen Imeev wrote:
> Hi! Thank you for review. My answers and new patch below.
> 
> On Fri, Mar 27, 2020 at 04:46:04PM +0000, Nikita Pettik wrote:
> > On 27 Mar 14:33, imeevma@tarantool.org wrote:
> > 
> > Could you please find Peter's table containing current/expected cast
> > behaviours and verify that this patch doesn't contradict it?
> > 
Here:
https://github.com/tarantool/doc/blob/pgulutzan-2.3/doc/reference/reference_sql/sql.rst

~                To BOOLEAN | To INTEGER | To NUMBER | To STRING | To VARBINARY
---------------  ----------   ----------   ---------   ---------   ------------
From BOOLEAN   | AAA        | A--        | ---       | A--       | ---
From INTEGER   | A--        | AAA        | AAA       | AAA       | ---
From NUMBER    | A--        | SSA        | AAA       | AAA       | ---
From STRING    | S--        | SSS        | SSS       | AAA       | A--
From VARBINARY | ---        | ---        | ---       | A--       | AAA

Should be fine since we have S for CAST() and implicit
cast from STRING to INTEGER.

> > > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> > > index aad030d..de1d9c3 100644
> > > --- a/src/box/sql/vdbemem.c
> > > +++ b/src/box/sql/vdbemem.c
> > > @@ -696,7 +696,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
> > >  		return -1;
> > >  	case FIELD_TYPE_INTEGER:
> > >  	case FIELD_TYPE_UNSIGNED:
> > > -		if ((pMem->flags & MEM_Blob) != 0) {
> > > +		if ((pMem->flags & (MEM_Blob | MEM_Str)) != 0) {
> > >  			bool is_neg;
> > >  			int64_t val;
> > >  			if (sql_atoi64(pMem->z, &val, &is_neg, pMem->n) != 0)
> > > @@ -711,8 +711,20 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
> > >  			MemSetTypeFlag(pMem, MEM_UInt);
> > >  			return 0;
> > >  		}
> > > -		if (sqlVdbeMemIntegerify(pMem) != 0)
> > > +		if ((pMem->flags & MEM_Real) != 0) {
> > > +			double d;
> > > +			if (sqlVdbeRealValue(pMem, &d) != 0)
> > > +				return -1;
> > > +			if (d < INT64_MAX && d >= INT64_MIN) {
> > > +				mem_set_int(pMem, d, d <= -1);
> > > +				return 0;
> > > +			}
> > > +			if (d >= INT64_MAX && d < UINT64_MAX) {
> > > +				mem_set_u64(pMem, d);
> > > +				return 0;
> > > +			}
> > >  			return -1;
> > 
> > Instead of keeping inlining code into sqlVdbeMemCast() I'd better
> > split it into separate functions. Good refactoring task tho - just
> > keep in mind.
> > 
> Thanks. I will fix this in #3809 issue.
> 
> > > +		}
> > >  		if (type == FIELD_TYPE_UNSIGNED &&
> > >  		    (pMem->flags & MEM_UInt) == 0)
> > >  			return -1;
> > > diff --git a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
> > > new file mode 100755
> > > index 0000000..0865c4e
> > > --- /dev/null
> > > +++ b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
> > 
> > Strictly speaking this change is not related to 4766, so I'd not
> > create separate test file for current patch. Moreover, tests in
> > sql/types.test.lua seem to cover it.
> > 
> I removed this file.
> 
> > > +#!/usr/bin/env tarantool
> > > +test = require("sqltester")
> > > +test:plan(6)
> > > +
> > > +--
> > > +-- Make sure that STRING or BLOB that contains DOUBLE value cannot
> > > +-- be cast to INTEGER.
> > > +--
> > > +test:do_catchsql_test(
> > > +    "gh-4766-1",
> > > +    [[
> > > +        SELECT CAST('1.1' AS INTEGER)
> > > +    ]], {
> > > +        1, "Type mismatch: can not convert 1.1 to integer"
> > > +    })
> > > +
> > > +test:do_catchsql_test(
> > > +    "gh-4766-2",
> > > +    [[
> > > +        SELECT CAST(x'312e31' AS INTEGER)
> > > +    ]], {
> > > +        1, "Type mismatch: can not convert varbinary to integer"
> > > +    })
> > > +
> > > diff --git a/test/sql/types.result b/test/sql/types.result
> > > index 38e4385..54aff46 100644
> > > --- a/test/sql/types.result
> > > +++ b/test/sql/types.result
> > > @@ -269,11 +269,8 @@ box.space.T1:drop()
> > >  --
> > >  box.execute("SELECT CAST('1.123' AS INTEGER);")
> > >  ---
> > > -- metadata:
> > > -  - name: CAST('1.123' AS INTEGER)
> > > -    type: integer
> > > -  rows:
> > > -  - [1]
> > > +- null
> > > +- 'Type mismatch: can not convert 1.123 to integer'
> > >  ...
> > >  box.execute("CREATE TABLE t1 (f TEXT PRIMARY KEY);")
> > >  ---
> > > @@ -285,13 +282,8 @@ box.execute("INSERT INTO t1 VALUES('0.0'), ('1.5'), ('3.9312453');")
> > >  ...
> > >  box.execute("SELECT CAST(f AS INTEGER) FROM t1;")
> > >  ---
> > > -- metadata:
> > > -  - name: CAST(f AS INTEGER)
> > > -    type: integer
> > > -  rows:
> > > -  - [0]
> > > -  - [1]
> > > -  - [3]
> > > +- null
> > > +- 'Type mismatch: can not convert 0.0 to integer'
> > >  ...
> > >  box.space.T1:drop()
> > >  ---
> > > @@ -1105,8 +1097,11 @@ box.execute("SELECT CAST(1.5 AS UNSIGNED);")
> > >  ...
> > >  box.execute("SELECT CAST(-1.5 AS UNSIGNED);")
> > >  ---
> > > -- null
> > > -- 'Type mismatch: can not convert -1 to unsigned'
> > > +- metadata:
> > > +  - name: CAST(-1.5 AS UNSIGNED)
> > > +    type: unsigned
> > > +  rows:
> > > +  - [-1]
> > >  ...
> > >  box.execute("SELECT CAST(true AS UNSIGNED);")
> > >  ---
> > > -- 
> > > 2.7.4
> > > 
> 
> 
> New patch:
> 
> 
> From d2679859651aeee2dffda01545e6c62ae3c185d1 Mon Sep 17 00:00:00 2001
> From: Mergen Imeev <imeevma@gmail.com>
> Date: Mon, 16 Mar 2020 15:12:37 +0300
> Subject: [PATCH] sql: fix CAST() from STRING to INTEGER
> 
> Prior to this patch, STRING, which contains the DOUBLE value,
> could be cast to INTEGER. This was done by converting STRING to
> DOUBLE and then converting this DOUBLE value to INTEGER. This may
> affect the accuracy of CAST(), so it was forbidden.
> 
> Before patch:
> box.execute("SELECT CAST('111.1' as INTEGER);")
> Result: 111
> 
> After patch:
> box.execute("SELECT CAST('1.1' as INTEGER);")
> Result: 'Type mismatch: can not convert 1.1 to integer'
> 
> box.execute("SELECT CAST('1.0' as INTEGER);")
> Result: 'Type mismatch: can not convert 1.0 to integer'
> 
> box.execute("SELECT CAST('1.' as INTEGER);")
> Result: 'Type mismatch: can not convert 1. to integer'
> 
> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> index aad030d..de1d9c3 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
> @@ -696,7 +696,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
>  		return -1;
>  	case FIELD_TYPE_INTEGER:
>  	case FIELD_TYPE_UNSIGNED:
> -		if ((pMem->flags & MEM_Blob) != 0) {
> +		if ((pMem->flags & (MEM_Blob | MEM_Str)) != 0) {
>  			bool is_neg;
>  			int64_t val;
>  			if (sql_atoi64(pMem->z, &val, &is_neg, pMem->n) != 0)
> @@ -711,8 +711,20 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
>  			MemSetTypeFlag(pMem, MEM_UInt);
>  			return 0;
>  		}
> -		if (sqlVdbeMemIntegerify(pMem) != 0)
> +		if ((pMem->flags & MEM_Real) != 0) {
> +			double d;
> +			if (sqlVdbeRealValue(pMem, &d) != 0)
> +				return -1;
> +			if (d < INT64_MAX && d >= INT64_MIN) {
> +				mem_set_int(pMem, d, d <= -1);
> +				return 0;
> +			}
> +			if (d >= INT64_MAX && d < UINT64_MAX) {
> +				mem_set_u64(pMem, d);
> +				return 0;
> +			}
>  			return -1;
> +		}
>  		if (type == FIELD_TYPE_UNSIGNED &&
>  		    (pMem->flags & MEM_UInt) == 0)
>  			return -1;
> diff --git a/test/sql/types.result b/test/sql/types.result
> index 38e4385..54aff46 100644
> --- a/test/sql/types.result
> +++ b/test/sql/types.result
> @@ -269,11 +269,8 @@ box.space.T1:drop()
>  --
>  box.execute("SELECT CAST('1.123' AS INTEGER);")
>  ---
> -- metadata:
> -  - name: CAST('1.123' AS INTEGER)
> -    type: integer
> -  rows:
> -  - [1]
> +- null
> +- 'Type mismatch: can not convert 1.123 to integer'
>  ...
>  box.execute("CREATE TABLE t1 (f TEXT PRIMARY KEY);")
>  ---
> @@ -285,13 +282,8 @@ box.execute("INSERT INTO t1 VALUES('0.0'), ('1.5'), ('3.9312453');")
>  ...
>  box.execute("SELECT CAST(f AS INTEGER) FROM t1;")
>  ---
> -- metadata:
> -  - name: CAST(f AS INTEGER)
> -    type: integer
> -  rows:
> -  - [0]
> -  - [1]
> -  - [3]
> +- null
> +- 'Type mismatch: can not convert 0.0 to integer'
>  ...
>  box.space.T1:drop()
>  ---
> @@ -1105,8 +1097,11 @@ box.execute("SELECT CAST(1.5 AS UNSIGNED);")
>  ...
>  box.execute("SELECT CAST(-1.5 AS UNSIGNED);")
>  ---
> -- null
> -- 'Type mismatch: can not convert -1 to unsigned'
> +- metadata:
> +  - name: CAST(-1.5 AS UNSIGNED)
> +    type: unsigned
> +  rows:
> +  - [-1]
>  ...
>  box.execute("SELECT CAST(true AS UNSIGNED);")
>  ---

  reply	other threads:[~2020-04-10 10:43 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-27 11:33 [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB " imeevma
2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING " imeevma
2020-03-27 16:46   ` Nikita Pettik
2020-04-10 10:39     ` Mergen Imeev
2020-04-10 10:43       ` Mergen Imeev [this message]
2020-04-10 13:05         ` Nikita Pettik
2020-04-10 17:06           ` Imeev Mergen
2020-04-15 11:13             ` Nikita Pettik
2020-04-10 12:46       ` Nikita Pettik
2020-04-10 13:05         ` Imeev Mergen
2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast " imeevma
2020-03-27 16:54   ` Nikita Pettik
2020-04-10 10:41     ` Mergen Imeev
2020-04-10 12:57       ` Nikita Pettik
2020-04-10 18:09         ` Mergen Imeev
2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 3/3] sql: add '\0' to the BLOB when it is cast " imeevma
2020-03-27 16:54   ` Nikita Pettik
2020-04-16  0:03 ` [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB " Nikita Pettik

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=20200410104352.GA20241@tarantool.org \
    --to=imeevma@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER' \
    /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