Re: [Tarantool-patches] [PATCH v3 2/8] sql: change implicit cast for assignment

[Nikita Pettik <korablev@tarantool.org>]

On 25 Jun 18:17, imeevma@tarantool.org wrote:
> On 22.06.2020 11:48, Nikita Pettik wrote:
> > On 17 Jun 15:36, imeevma@tarantool.org wrote:
> >> This patch removes type changing from OP_MakeRecord.
> >
> > Please reflect user-visible changes in commit message.
> >  
> Fixed.

Seems to be the next patch..
 
> On 22.06.2020 12:32, Nikita Pettik wrote:
> > On 17 Jun 15:36, imeevma@tarantool.org wrote:
> >> This patch removes implicit cast from STRING to numeric
> >> and vice versa of left operand of IN operator.
> >>
> >> Part of #4230
> >> Part of #4692
> >> ---
> >>  src/box/sql/expr.c                   |  2 +-
> >>  test/sql-tap/in3.test.lua            | 14 +-----
> >>  test/sql-tap/subquery.test.lua       | 69 +---------------------------
> >>  test/sql-tap/tkt-80e031a00f.test.lua |  4 +-
> >>  test/sql/boolean.result              | 12 ++---
> >>  5 files changed, 11 insertions(+), 90 deletions(-)
> 
> On 22.06.2020 13:07, Nikita Pettik wrote:
> > On 17 Jun 15:36, imeevma@tarantool.org wrote:
> >> This patch replaces mem_apply_type() with mem_check_types() in
> >> OP_MustBeInt, which allows to remove implicit cast in some places,
> >> for example, in the IN statement.
> >>
> >> Part of #4230
> >> ---
> >>  src/box/sql/vdbe.c                   |   2 +-
> >>  test/sql-tap/e_select1.test.lua      |  17 +-
> >>  test/sql-tap/in4.test.lua            |  97 +--
> >>  test/sql-tap/join.test.lua           |   1 -
> >>  test/sql-tap/limit.test.lua          |   2 +-
> >>  test/sql-tap/tkt-9a8b09f8e6.test.lua |  24 +-
> >>  test/sql-tap/tkt-fc7bd6358f.test.lua | 111 ----
> >>  test/sql-tap/transitive1.test.lua    |   4 +-
> >>  test/sql-tap/whereB.test.lua         | 900 ---------------------------
> >>  test/sql-tap/whereC.test.lua         |   8 +-
> >>  10 files changed, 19 insertions(+), 1147 deletions(-)
> >>  delete mode 100755 test/sql-tap/tkt-fc7bd6358f.test.lua
> >>  delete mode 100755 test/sql-tap/whereB.test.lua
> >>
> 
> New patch:
> 
> commit 21d7145c1929bc4606c56e9a566477f248637ed1
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Wed May 27 13:49:11 2020 +0300
> 
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 37283e506..f1d0345f9 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -397,6 +397,15 @@ sql_value_to_diag_str(sql_value *value);
>  enum mp_type
>  sql_value_type(sql_value *);
>  
> +/*
> + * Return the MP_type of the value of the MEM.
> + *
> + * @param mem MEM with the correct MEM_type.
> + * @retval MP_type of the value.
> + */
> +enum mp_type
> +sql_value_mp_type(struct Mem *mem);

It's not sql_value *, it's struct Mem. So the name of function
is misleading.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 950f72ddd..009d577d2 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -417,6 +417,143 @@ sql_value_apply_type(
>   mem_apply_type((Mem *) pVal, type);
>  }
>  
> + *
> + * @param mem The MEM that contains the value to check.
> + * @param type The type to check.
> + * @retval 0 if the MEM_type of the value ​​and the given type are

Wrong symbols.

> + *         compatible, -1 otherwise.
> + */
> +static int
> +mem_check_type(struct Mem *mem, enum field_type type)

mem_test_type_compatibility
mem_check_type_consistency
mem_is_type_compatible

I'd prefer last one and change return value to boolean.
Then it turns into simple return field_mp_plain_type_is_compatible()..

> + enum mp_type mp_type = sql_value_mp_type(mem);
> + assert(mp_type < MP_EXT);
> + if (field_mp_plain_type_is_compatible(type, mp_type, true))
> +   return 0;
> + return -1;
> +}
> +
> +/**
> + * Convert the numeric value contained in MEM to double. If the
> + * is_precise flag is set, the conversion will succeed only if it
> + * is lossless.
> + *
> + * @param mem The MEM that contains the numeric value.
> + * @param is_precise Flag.
> + * @retval 0 if the conversion was successful, -1 otherwise.
> + */
> +static int
> +mem_convert_to_double(struct Mem *mem, bool is_precise)
> +{
> + if ((mem->flags & (MEM_Int | MEM_UInt)) == 0)
> +   return -1;
> + if ((mem->flags & MEM_Int) != 0) {
> +   int64_t i = mem->u.i;
> +   double d = (double)i;
> +   if (!is_precise || i == (int64_t)d)
> +     mem_set_double(mem, d);
> +   else
> +     return -1;
> + } else {
> +   uint64_t u = mem->u.u;
> +   double d = (double)u;
> +   if (!is_precise || u == (uint64_t)d)
> +     mem_set_double(mem, d);
> +   else
> +     return -1;
> + }
> + mem->field_type = FIELD_TYPE_DOUBLE;

Why not incorparate field_type assingment into mem_set_double()?
The same concerns other converting functions.

> +/**
> + * Convert the numeric value contained in MEM to another numeric
> + * type. If the is_precise flag is set, the conversion will
> + * succeed only if it is lossless.
> + *
> + * @param mem The MEM that contains the numeric value.
> + * @param type The type to convert to.
> + * @param is_precise Flag.
> + * @retval 0 if the conversion was successful, -1 otherwise.
> + */
> +static int
> +mem_convert_numeric(struct Mem *mem, enum field_type type, bool is_precise)

mem_convert_to_numeric ?
mem_cast_to_numeric

> +{
> + if (!sql_type_is_numeric(type) ||
> +     (mem->flags & (MEM_Real | MEM_Int | MEM_UInt)) == 0)

It's somehow unnatural passing to _numeric function non-numeric type
to convert to. Instead let's use this function properly and replace
branching with assertion.

> +   return -1;
> + if (type == FIELD_TYPE_NUMBER)
> +   return 0;
> + if (type == FIELD_TYPE_DOUBLE)
> +   return mem_convert_to_double(mem, is_precise);
> + if (type == FIELD_TYPE_UNSIGNED)
> +   return mem_convert_to_unsigned(mem, is_precise);
> + assert(type == FIELD_TYPE_INTEGER);
> + return mem_convert_to_integer(mem, is_precise);
> +}
> +
>  /*
>   * pMem currently only holds a string type (or maybe a BLOB that we can
>   * interpret as a string if we want to).  Compute its corresponding
> @@ -2773,6 +2910,36 @@ case OP_ApplyType: {
>   break;
>  }
>  
> +/* Opcode: CheckType P1 P2 * P4 *
> + * Synopsis: type(r[P1@P2])
> + *
> + * Check that types of P2 registers starting from register
> + * P1 are compatible with given with given field types in P4.

-> double  'with given'

> + * If the MEM_type of the value and the given type are
> + * incompatible, but both are numeric, this opcode attempts to

Incompatible but numeric? Could you please be more specific.

> + * convert the value to the type.
> + */
> +case OP_ImplicitCast: {
> + enum field_type *types = pOp->p4.types;
> + assert(types != NULL);
> + assert(types[pOp->p2] == field_type_MAX);
> + pIn1 = &aMem[pOp->p1];
> + enum field_type type;
> + while((type = *(types++)) != field_type_MAX) {
> +   assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]);
> +   assert(memIsValid(pIn1));
> +   if (mem_check_type(pIn1, type) != 0 &&
> +       mem_convert_numeric(pIn1, type, false) != 0) {
> +     diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> +        sql_value_to_diag_str(pIn1),
> +        field_type_strs[type]);
> +     goto abort_due_to_error;
> +   }
> +   pIn1++;
> + }