[Tarantool-patches] [PATCH v5 21/52] sql: introduce bitwise operations for MEM

[Mergen Imeev]

Thank you for the review! My answers below.

On Tue, Apr 13, 2021 at 01:31:39AM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> See 2 comments below.
> 
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index eee72a7fe..aeb801c7c 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -624,6 +624,115 @@ mem_rem(const struct Mem *left, const struct Mem *right, struct Mem *result)
> 
> <...>
> 
> > +
> > +int
> > +mem_shift_right(const struct Mem *left, const struct Mem *right,
> > +   struct Mem *result)
> > +{
> > + if (is_result_null(left, right, result, FIELD_TYPE_INTEGER))
> > +   return 0;
> > + int64_t a;
> > + int64_t b;
> > + if (bitwise_prepare(left, right, &a, &b) != 0)
> > +   return -1;
> > + if (b <= -64)
> > +   result->u.i = 0;
> > + else if (b < 0)
> > +   result->u.i = a << -b;
> > + else if (b > 64)
> > +   result->u.i = a >= 0 ? 0 : -1;
> > + else
> > +   result->u.i = a >> b;
> 
> 1. Right shit has different meaning for negative and positive
> numbers. This code produces invalid output:
> 
> 	tarantool> box.execute('SELECT 9223372036854775808 >> 3')
> 	---
> 	- metadata:
> 	  - name: COLUMN_1
> 	    type: integer
> 	  rows:
> 	  - [-1152921504606846976]
> 	...
> 
> The number should have decreased, but it should not have changed
> its sign. It should be 1152921504606846976, positive. But I see
> the same bug exists on the master branch, even though it had
> special handling for negative numbers, which is also broken.
> 
> Is there a ticket for that? Can you fix it right away?
> 
There is one issue about these operations: #5364. I think these bugs shuld be
fixed in this issue.

> > + result->flags = result->u.i < 0 ? MEM_Int : MEM_UInt;
> > + return 0;
> > +}
> > +
> > +int
> > +mem_bit_not(const struct Mem *mem, struct Mem *result)
> > +{
> > + mem_clear(result);
> > + result->field_type = FIELD_TYPE_INTEGER;
> > + if ((mem->flags & MEM_Null) != 0)
> > +   return 0;
> > + int64_t i;
> > + bool unused;
> > + if (sqlVdbeIntValue(mem, &i, &unused) != 0) {
> > +   diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(mem),
> > +      "integer");
> > +   return -1;
> > + }
> > + result->u.i = ~i;
> > + result->flags = result->u.i < 0 ? MEM_Int : MEM_UInt;
> 
> 2. What if the original value was positive, and the user also
> expected to get a positive result? For example, what if he did
> ~CAST(value AS UNSIGNED)? Or is it useless and I am expected to
> do CAST(~value as UNSIGNED)?
I believe there is no way to get unsigned value more that INT64_MAX. Casts also
won't work:

tarantool> box.execute('select ~1;')
---
- metadata:
  - name: COLUMN_1
    type: integer
  rows:
  - [-2]
...

tarantool> box.execute('select CAST(~1 AS UNSIGNED);')
---
- null
- 'Type mismatch: can not convert -2 to unsigned'
...