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

[Nikita Pettik <korablev@tarantool.org>]

On 05 Jul 17:26, Mergen Imeev wrote:
> Hi! Thank you for the review. My answers and new patch
> below. Sorry, I didn't include diff.
> > > commit 21d7145c1929bc4606c56e9a566477f248637ed1
> > > Author: Mergen Imeev <imeevma@gmail.com>
> > > Date:   Wed May 27 13:49:11 2020 +0300
> > > 
> > > + 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.
> > 
> I think this should be done in #4906 issue since it may
> lead to not always simple to understand changes.

Эм, не очень понял как трактовать твой ответ. Типо тут работает,
а если там засунуть, то что-то начинает падать..? 4906 немного про
другое: в любом случае у нас в меме будет и field_type и mp_type.
К тому же, и то, и это - рефакторинг и функциональных изменений быть
не должно.
 
> > > +/**
> > > + * 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
> > 
> Used first option.
> 
> > > +{
> > > + 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.
> > 
> No, I think it is good as it is. More that that, due to
> some other comments I added similar checks to all other
> mem_convert_to>* functions.

В остальных случаях ты туда тип вообще не передаешь.
 
> My reason is: in all places we use this function we have
> to check that field type and MEM_type are numeric, so it
> make sence to move this check inside of the function.

Еще раз: это полный идиотизм передавать ТИП (отличный от нумерика) к
которому ты собираешься кастовать в функцию, которая по названию
предполагает каст к нумерику. Считай, что я настаиваю на этом
изменении.

> > > +   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);
> > > +}
> > > +
> > >  /*
> index 37283e506..bd0055668 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
> +mem_mp_type(struct Mem *mem);

Я вытащил добавление этой функции в отдельный патч и добавил
рефакторинг чтобы соеденить ее с sql_value_type().
https://github.com/tarantool/tarantool/tree/np/gh-3809-remastered

Если ты ОК, я пушну вне очереди в мастер.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 950f72ddd..1386ea2c0 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> +		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;
> +	return 0;
> +}
> +
> + * P1 are compatible with given field types in P4.
> + * If the MEM_type of the value and the given type are
> + * incompatible according to field_mp_plain_type_is_compatible(),
> + * but both are numeric, this opcode attempts to 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);
> +	for (int i = 0; types[i] != field_type_MAX; ++i) {
> +		enum field_type type = types[i];
> +		pIn1 = &aMem[pOp->p1 + i];
> +		assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]);
> +		assert(memIsValid(pIn1));
> +		if (mem_is_type_compatible(pIn1, type))
> +			continue;
> +		if (mem_convert_to_numeric(pIn1, type, false) == 0)

Инвертируй, пожайлуйста условие:

if (mem_convert_to_numeric(pIn1, type, false) !=) {
	//raise an error
}

Это будет больще соответствовать нашему кодстайлу.

> +			continue;
> +		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> +			 sql_value_to_diag_str(pIn1), field_type_strs[type]);
> +		goto abort_due_to_error;
> +	}
> +	break;
> +}
> +
>  /* Opcode: MakeRecord P1 P2 P3 P4 P5
>   * Synopsis: r[P3]=mkrec(r[P1@P2])
>   *
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index 4e103a653..b106ce6bd 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -225,6 +225,33 @@ sql_value_type(sql_value *pVal)
>  	}
>  }
>  
> +enum mp_type
> +mem_mp_type(struct Mem *mem)

Еще раз: это не апишная функция (хотя бы в терминологии скулайта),
ей место скорее в vdbemem.c. См. мой рефакторинг.

> +{
> --- a/test/sql-tap/autoinc.test.lua
> +++ b/test/sql-tap/autoinc.test.lua

Тесты не смотрел, предполагая что нет изменений по сравнению с
предыдущей версией.