Re: [Tarantool-patches] [PATCH v5 19/52] sql: introduce arithmetic operations for MEM

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the fixes!

>>> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
>>> index 2d76ef88d..859e337aa 100644
>>> --- a/src/box/sql/mem.c
>>> +++ b/src/box/sql/mem.c
>>> @@ -390,6 +390,240 @@ mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
>>> +
>>> +static int
>>> +get_number(const struct Mem *mem, struct sql_num *number)
>>> +{
>>> + if ((mem->flags & MEM_Real) != 0) {
>>> +   number->d = mem->u.r;
>>> +   number->type = MEM_Real;
>>> +   return 0;
>>> + }
>>> + if ((mem->flags & MEM_Int) != 0) {
>>> +   number->i = mem->u.i;
>>> +   number->type = MEM_Int;
>>> +   number->is_neg = true;
>>> +   return 0;
>>> + }
>>> + if ((mem->flags & MEM_UInt) != 0) {
>>> +   number->u = mem->u.u;
>>> +   number->type = MEM_UInt;
>>> +   number->is_neg = false;
>>> +   return 0;
>>> + }
>>> + if ((mem->flags & (MEM_Str | MEM_Blob)) == 0)
>>> +   return -1;
>>> + if ((mem->flags & MEM_Subtype) != 0)
>>> +   return -1;
>>> + if (sql_atoi64(mem->z, &number->i, &number->is_neg, mem->n) == 0) {
>>> +   number->type = number->is_neg ? MEM_Int : MEM_UInt;
>>> +   /*
>>> +    * The next line should be removed along with the is_neg field
>>> +    * of struct sql_num. The integer type tells us about the sign.
>>> +    * However, if it is removed, the behavior of arithmetic
>>> +    * operations will change.
>>> +    */
>>> +   number->is_neg = (mem->flags & MEM_Int) != 0;
>>
>> I don't understand that. How is it possible it mismatches the
>> value returned from sql_atoi64()? And why isn't it just 'false' then?
>> Because a few lines above you already checked (mem->flags & MEM_Int) != 0
>> and it was false.
>>
> Not exactly right. For example:
> 
> tarantool> box.execute([[SELECT '-5' + 2;]])
> ---
> - metadata:
>   - name: COLUMN_1
>     type: integer
>   rows:
>   - [18446744073709551613]
> ...
> 
> As you see, this is wrong. This is due to the fact, that MEM of type string do
> not have MEM_Int set. Even though this is wrong, it is expected behaviour. I
> created an issue for this: #5756. Since I didn't want to change this behaviour,
> I added is_neg field to struct sql_num. This is clearly a hack and should be
> fixed.

But that does not answer the second part of my question - why can't
I set it to false here always?

====================
@@ -286,7 +286,7 @@ get_number(const struct Mem *mem, struct sql_num *number)
 		 * However, if it is removed, the behavior of arithmetic
 		 * operations will change.
 		 */
-		number->is_neg = (mem->flags & MEM_Int) != 0;
+		number->is_neg = false;
 		return 0;
 	}
====================

Because (mem->flags & MEM_Int) == 0, otherwise it would return earlier above.

Also 'is_neg' is not used at all now in all places where get_number() is called.
At least in this commit. I would propose to add it in the commit which needs it
or remove it then now.