Re: [Tarantool-patches] [PATCH v2 1/1] sql: limit blob size during CAST AS INTEGER

[Mergen Imeev <imeevma@tarantool.org>]

Hi! Thank you for review. I changed a test once again.
Diff below.

On Thu, Feb 20, 2020 at 10:58:21PM +0300, Nikita Pettik wrote:
> On 13 Feb 11:16, imeevma@tarantool.org wrote:
> > >> +test:do_execsql_test(
> > >> +    "cast-6.1",
> > >> +    [[
> > >> +        CREATE TABLE t (a VARBINARY PRIMARY KEY);
> > >> +        INSERT INTO t VALUES (X'33'), (X'372020202020');
> > >> +        SELECT a, CAST(a AS NUMBER), CAST(a AS INTEGER), CAST(a AS UNSIGNED) FROM t;
> > >> +        DROP TABLE t;
> > >> +    ]], {
> > >> +        -- <cast-6.1>
> > >> +        '3', 3, 3, 3, '7     ', 7, 7, 7
> > >> +        -- </cast-6.1>
> > >> +    })
> > >> +
> > >> +test:do_execsql_test(
> > >> +    "cast-6.2",
> > >> +    [[
> > >> +        CREATE TABLE t (a VARBINARY PRIMARY KEY, i INT);
> > >> +        INSERT INTO t VALUES (X'33', 1);
> > >> +        SELECT a, CAST(a AS NUMBER), CAST(a AS INTEGER), CAST(a AS UNSIGNED) FROM t;
> > >> +        DROP TABLE t;
> > >
> > > What's the (in functional sense) difference between 6.1 and 6.2?
> > >
> > True, in this test it isn't obvious what it should show. I fixed
> > the test. I made this test to show that the '\0' at the end of the
> > BLOB is important. One possible way to solve this problem was to
> > limit the length of the decoded BLOB (without copying and adding
> > '\0'), but this could lead to an incorrect result of this test.
> > I mean this:
> > 
> > tarantool> CREATE TABLE t (a VARBINARY PRIMARY KEY, i INT) WITH ENGINE = 'vinyl';
> > ---
> > - row_count: 1
> > ...
> > 
> > tarantool> INSERT INTO t VALUES (X'33', 0x33);
> > ---
> > - row_count: 1
> > ...
> 
> So now you insert 0x33 instead of 1 to integer field. But how does it
> affect test? I failed to understand. In both cases you fetch and operate
> on blob, meanwhile integer field doesn't seem to be involved.
> 
As I wrote in the last letter, we have a way to make sure
that with the first case everything will be in order,
without creating a duplicate of this binary value.
Obviously, that method will definitely not affect
performance. But it can lead to the part of the value that
looks like X'333300' being decoded as 33. See the example
from the last letter.

> > 
> > tarantool> SELECT a, CAST(a AS NUMBER), CAST(a AS INTEGER), CAST(a AS UNSIGNED) FROM t;
> > ---
> > - metadata:
> >   - name: A
> >     type: varbinary
> >   - name: CAST(a AS NUMBER)
> >     type: number
> >   - name: CAST(a AS INTEGER)
> >     type: integer
> >   - name: CAST(a AS UNSIGNED)
> >     type: unsigned
> >   rows:
> >   - ['3', 33, 33, 33]
> > ...
> > 
> > +--
> > +-- In some cases, the absence of '\0' could lead to an incorrect
> > +-- result. Make sure this does not happen now.
> > +--
> >  test:do_execsql_test(
> >      "cast-6.2",
> >      [[
> >          CREATE TABLE t (a VARBINARY PRIMARY KEY, i INT);
> > -        INSERT INTO t VALUES (X'33', 1);
> > +        INSERT INTO t VALUES (X'33', 0x33);
> >          SELECT a, CAST(a AS NUMBER), CAST(a AS INTEGER), CAST(a AS UNSIGNED) FROM t;
> >          DROP TABLE t;
> >      ]], {
> > 

Diff:

diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua
index 86c0fee..74844e0 100755
--- a/test/sql-tap/cast.test.lua
+++ b/test/sql-tap/cast.test.lua
@@ -891,13 +891,15 @@ test:do_execsql_test(
 
 --
 -- In some cases, the absence of '\0' could lead to an incorrect
--- result. Make sure this does not happen now.
+-- result. For example, in this case, part of the value is as
+-- follows: X'333300', which can be decoded as the number 33. Make
+-- sure this does not happen now.
 --
 test:do_execsql_test(
     "cast-6.2",
     [[
-        CREATE TABLE t (a VARBINARY PRIMARY KEY, i INT);
-        INSERT INTO t VALUES (X'33', 0x33);
+        CREATE TABLE t (a VARBINARY PRIMARY KEY, i INT, u INT);
+        INSERT INTO t VALUES (X'33', 0x33, 0x00);
         SELECT a, CAST(a AS NUMBER), CAST(a AS INTEGER), CAST(a AS UNSIGNED) FROM t;
         DROP TABLE t;
     ]], {