[tarantool-patches] Re: [PATCH 6/9] sql: make comparison predicate return boolean

[n.pettik]

> On 16 Apr 2019, at 17:12, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
> 
> Thanks for the patch! See 3 comments below.
> 
> On 14/04/2019 18:04, Nikita Pettik wrote:
>> 
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index 10794b18e..43d4262e5 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -2273,8 +2273,8 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
>> 			if ((pOp->opcode==OP_Eq)==res2) break;
>> 		}
>> 		memAboutToChange(p, pOut);
>> -		MemSetTypeFlag(pOut, MEM_Int);
>> -		pOut->u.i = res2;
>> +		MemSetTypeFlag(pOut, MEM_Bool);
>> +		pOut->u.b = res2;
> 
> 1. Please, update the comments about SQL_STOREP2 and SQL_KEEPNULL.
> They still say the result is stored as int 1/0.

Do you mean these comments (?) :

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 43d4262e5..b28e9b4ea 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2061,8 +2061,8 @@ case OP_Cast: {                  /* in1 */
  * registers P1 and P3.
  *
  * If both SQL_STOREP2 and SQL_KEEPNULL flags are set then the
- * content of r[P2] is only changed if the new value is NULL or 0 (false).
- * In other words, a prior r[P2] value will not be overwritten by 1 (true).
+ * content of r[P2] is only changed if the new value is NULL or false.
+ * In other words, a prior r[P2] value will not be overwritten by true.
  */
 /* Opcode: Ne P1 P2 P3 P4 P5
  * Synopsis: IF r[P3]!=r[P1]
@@ -2072,15 +2072,15 @@ case OP_Cast: {                  /* in1 */
  * additional information.
  *
  * If both SQL_STOREP2 and SQL_KEEPNULL flags are set then the
- * content of r[P2] is only changed if the new value is NULL or 1 (true).
- * In other words, a prior r[P2] value will not be overwritten by 0 (false).
+ * content of r[P2] is only changed if the new value is NULL or true.
+ * In other words, a prior r[P2] value will not be overwritten by false.
  */
 /* Opcode: Lt P1 P2 P3 P4 P5
  * Synopsis: IF r[P3]<r[P1]
  *
  * Compare the values in register P1 and P3.  If reg(P3)<reg(P1) then
  * jump to address P2.  Or if the SQL_STOREP2 flag is set in P5 store
- * the result of comparison (0 or 1 or NULL) into register P2.
+ * the result of comparison (false or true or NULL) into register P2.
  *
  * If the SQL_JUMPIFNULL bit of P5 is set and either reg(P1) or
  * reg(P3) is NULL then the take the jump.  If the SQL_JUMPIFNULL

> 
>> 		REGISTER_TRACE(pOp->p2, pOut);
>> 	} else {
>> 		VdbeBranchTaken(res!=0, (pOp->p5 & SQL_NULLEQ)?2:3);
>> diff --git a/test/sql-tap/aggnested.test.lua b/test/sql-tap/aggnested.test.lua
>> index 656576b70..67a9ba891 100755
>> --- a/test/sql-tap/aggnested.test.lua
>> +++ b/test/sql-tap/aggnested.test.lua
>> @@ -215,7 +215,7 @@ test:do_execsql_test("aggnested-3.2-2",
>>     ]],
>>     {
>>         -- <aggnested-3.2>
>> -        0
>> +        ""
> 
> 2. Why? There 'sum' was selected. It can not return text.

I guess because nulls are converted this way in execsql function.
SUM() now is not called at all (in previous test version it was called
with 0 as arg), so query returns NULL.

>> @@ -227,13 +227,13 @@ test:do_execsql_test("aggnested-3.3",
>>         INSERT INTO t1 VALUES(4469,2),(4469,1);
>>         CREATE TABLE t2 (value2 INT PRIMARY KEY);
>>         INSERT INTO t2 VALUES(1);
>> -        SELECT (SELECT sum(value2=value1) FROM t2), max(value1)
>> +        SELECT (SELECT sum(value1) FROM t2 where value1=value2), max(value1)
>>           FROM t1
>>          GROUP BY id1;
>>     ]],
>>     {
>>         -- <aggnested-3.3>
>> -        0, 2
>> +        "", 2
> 
> 3. The same.
> 
>>         -- </aggnested-3.3>
>>     })
>>