Re: [Tarantool-patches] [PATCH v5 6/6] sql: remove implicit cast from MakeRecord opcode

[Mergen Imeev <imeevma@tarantool.org>]

Thank you for review! My answers and diff below.

On Fri, Sep 18, 2020 at 12:28:10PM +0000, Nikita Pettik wrote:
> On 21 Aug 12:19, imeevma@tarantool.org wrote:
> > This patch removes implicit casting from MakeRecord opcode.
> > 
> > Closes #4230
> > 
> > @TarantoolBot document
> > Title: remove implicit cast for comparison
> > 
> > After this patch-set, there will be no implicit casts for comparison.
> > This means that the values ​of the field types STRING, BOOLEAN and
> 
> Again these strange characters in commit msg..
Sorry, fixed.

> 
> > VARBINARY can be compared with the values of the same field type.
> 
> Only.
Fixed:

Title: remove implicit cast for comparison

After this patch-set, there will be no implicit casts for comparison.
This means that the values of field types STRING, BOOLEAN, and
VARBINARY can only be compared with the values of the same field type.
Any numeric value can be compared to any other numeric value.

> 
> > Any numerical value can be compared with any other numerical value.
> > 
> > Example:
> > 
> > ```
> > tarantool> box.execute([[SELECT '1' > 0;]])
> > ---
> > - null
> > - 'Type mismatch: can not convert 1 to numeric'
> > ...
> > 
> > tarantool> box.execute([[SELECT true > X'33';]])
> > ---
> > - null
> > - 'Type mismatch: can not convert boolean to varbinary'
> > ...
> > 
> > tarantool> box.execute([[SELECT 1.23 > 123;]])
> > ---
> > - metadata:
> >   - name: 1.23 > 123
> >     type: boolean
> >   rows:
> >   - [false]
> > ...
> > ```
> > ---
> > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> > index bc2182446..d553b80db 100644
> > --- a/src/box/sql/expr.c
> > +++ b/src/box/sql/expr.c
> > @@ -2886,11 +2886,18 @@ sqlCodeSubselect(Parse * pParse,	/* Parsing context */
> >  						jmpIfDynamic = -1;
> >  					}
> >  					r3 = sqlExprCodeTarget(pParse, pE2, r1);
> > -					enum field_type types[2] =
> > -						{ lhs_type, field_type_MAX };
> > -	 				sqlVdbeAddOp4(v, OP_MakeRecord, r3,
> > -							  1, r2, (char *)types,
> > -							  sizeof(types));
> > +					uint32_t size =
> > +						2 * sizeof(enum field_type);
> > +					enum field_type *types=
> > +						sqlDbMallocZero(pParse->db,
> 
> Why do you have manually alloc types? It is done in sqlVdbeAddOp4()
> if pass n > 0 (like it was before your refactoring).
> 
You are right, fixed.

> > +								size);
> > +					types[0] = lhs_type;
> > +					types[1] = field_type_MAX;
> > +					sqlVdbeAddOp4(v, OP_ApplyType, r3, 1, 0,
> > +						      (char *)types, P4_DYNAMIC);
> > +					sqlVdbeChangeP5(v, OPFLAG_DO_NOT_CONVERT_NUMBERS);
> > +					sqlVdbeAddOp3(v, OP_MakeRecord, r3, 1,
> > +						      r2);
> >  					sql_expr_type_cache_change(pParse,
> >  								   r3, 1);
> >  					sqlVdbeAddOp2(v, OP_IdxInsert, r2,
> 
> You also can remove now mem_apply_type().
> 
No, mem_apply_type() used in one place sql_value_apply_type(), however this
function used in 8 more places. I think we need to do a bit more before we will
be able to remove mem_apply_type().

> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index a0ddbaf60..544619b03 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -3145,24 +3145,17 @@ type_mismatch:
> >  	break;
> >  }
> >  
> > diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua
> > index a6d842962..7f3abbae0 100755
> > --- a/test/sql-tap/in3.test.lua
> > +++ b/test/sql-tap/in3.test.lua
> > @@ -1,6 +1,6 @@
> >  #!/usr/bin/env tarantool
> >  test = require("sqltester")
> > -test:plan(28)
> > +test:plan(26)
> >  
> >  --!./tcltestrunner.lua
> >  -- 2007 November 29
> > @@ -334,18 +334,6 @@ test:do_test(
> >          -- </in3-3.4>
> >      })
> >  
> > -test:do_test(
> > -    "in3-3.5",
> > -    function()
> > -        -- Numeric affinity should be applied to each side before the comparison
> > -        -- takes place. Therefore we cannot use index t1_i1, which has no affinity.
> > -        return exec_neph(" SELECT y IN (SELECT a FROM t1) FROM t2 ")
> > -    end, {
> > -        -- <in3-3.5>
> > -        1, true
> > -        -- </in3-3.5>
> > -    })
> > -
> >  test:do_test(
> >      "in3-3.6",
> >      function()
> > @@ -358,18 +346,6 @@ test:do_test(
> >          -- </in3-3.6>
> >      })
> >  
> > -test:do_test(
> > -    "in3-3.7",
> > -    function()
> > -        -- Numeric affinity is applied before the comparison takes place. 
> > -        -- Making it impossible to use index t1_i3.
> > -        return exec_neph(" SELECT y IN (SELECT c FROM t1) FROM t2 ")
> > -    end, {
> > -        -- <in3-3.7>
> > -        1, true
> > -        -- </in3-3.7>
> > -    })
> > -
> 
> Why did you remove these tests?
Now they throw an error, so it is useless to have them here. We have other tests
to check these errors and they don't check usage of ephemeral space anymore.

> 
> >  -----------------------------------------------------------------------
> >  --
> >  -- Test using a multi-column index.


Diff:


diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index d553b80db..764e68cb1 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2886,15 +2886,11 @@ sqlCodeSubselect(Parse * pParse,	/* Parsing context */
 						jmpIfDynamic = -1;
 					}
 					r3 = sqlExprCodeTarget(pParse, pE2, r1);
-					uint32_t size =
-						2 * sizeof(enum field_type);
-					enum field_type *types=
-						sqlDbMallocZero(pParse->db,
-								size);
-					types[0] = lhs_type;
-					types[1] = field_type_MAX;
+					enum field_type types[2] =
+						{ lhs_type, field_type_MAX };
 					sqlVdbeAddOp4(v, OP_ApplyType, r3, 1, 0,
-						      (char *)types, P4_DYNAMIC);
+						      (char *)types,
+						      sizeof(types));
 					sqlVdbeChangeP5(v, OPFLAG_DO_NOT_CONVERT_NUMBERS);
 					sqlVdbeAddOp3(v, OP_MakeRecord, r3, 1,
 						      r2);