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

[Vladislav Shpilevoy]

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?

> + 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)?