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

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thank you for the review! My answers below.

On Sun, Apr 11, 2021 at 08:13:27PM +0200, Vladislav Shpilevoy wrote:
> Good job on the fixes!
> 
> >> The names could be mem_arith_plus(), mem_arith_mul(), mem_arith_minus(),
> >> etc.
> > Fixed. I named new functions mem_add(), mem_sub(), mem_mul(), mem_div() and
> > mem_rem(). Each of them simpler than this function.
> 
> The last operation is called modulo. Usually shortened to mod. But I see
> we already use 'rem' in some other place, and in the token name. Up to you.
> 
I will leave it as it is for now. This function should be fixed at some point
(see comments in its body).

> See 1 comment below.
> 
> > 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.


> > +   return 0;
> > + }
> > + if (sqlAtoF(mem->z, &number->d, mem->n) != 0) {
> > +   number->type = MEM_Real;
> > +   return 0;
> > + }
> > + return -1;
> > +}