Re: [Tarantool-patches] [PATCH v3 6/8] sql: remove implicit cast for comparison

[Nikita Pettik <korablev@tarantool.org>]

On 05 Jul 17:47, Mergen Imeev wrote:
> My answers and new patch below.
> 
> On Mon, Jun 29, 2020 at 11:51:29PM +0000, Nikita Pettik wrote:
> > On 25 Jun 18:17, imeevma@tarantool.org wrote:
> > > Thank you for review! My answers and new patch below.
> > > 
> > > On 22.06.2020 15:25, Nikita Pettik wrote:
> > > > On 17 Jun 15:36, imeevma@tarantool.org wrote:
> > > >> @@ -2399,14 +2387,6 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
> > > >>  	default:       res2 = res>=0;     break;
> > > >>  	}
> > > >>  
> > > >> -	/* Undo any changes made by mem_apply_type() to the input registers. */
> > > >> -	assert((pIn1->flags & MEM_Dyn) == (flags1 & MEM_Dyn));
> > > >> -	pIn1->flags = flags1;
> > > >> -	pIn1->field_type = ft_p1;
> > > >> -	assert((pIn3->flags & MEM_Dyn) == (flags3 & MEM_Dyn));
> > > >> -	pIn3->flags = flags3;
> > > >> -	pIn3->field_type = ft_p3;
> > > >> -
> > > >
> > > > Replace these assertions with relevant ones.
> > > >
> > > Not sure if this is necessary, since now we have much less flag
> > > changes after removal of mem_apply_type().
> > 
> > Не понял как это связано...Твои изменения не затрагивают
> > инвариант MEM_Dyn.
> >  
> True, however original asserts checked that
> mem_apply_type() does not changed it. Now we can say that
> it definitely won't be changed. I can return flags*
> variables for the sake of these asserts. Should I do this?
> 
> Also, after #4906 these asserts become outdated since flags
> of the mems won't change at all. Well, it will be so if we
> close #4906.

У этого тикета даже майлстоуна нет, если что.
 
> > > @@ -4744,7 +4753,25 @@ case OP_IdxGE:  {       /* jump */
> > >  		assert(pOp->opcode==OP_IdxGE || pOp->opcode==OP_IdxLT);
> > >  		r.default_rc = 0;
> > >  	}
> > > +
> > > +	/*
> > > +	 * Make sure that the types of all the fields in the tuple
> > > +	 * that will be used in the iterator comparable with the
> > > +	 * fields of the space.
> > > +	 */
> > >  	r.aMem = &aMem[pOp->p3];
> > > +	for (int i = 0; i < r.nField; ++i) {
> > > +		enum field_type type = r.key_def->parts[i].type;
> > > +		struct Mem *mem = &r.aMem[i];
> > 
> > Как-то мне эти два места (выше в SeekGE) вообще не нравятся.
> > Какие-то они запутанные. Давай подумаем как можно зарефакторить их.
> > 
> Moved to a new function. However, previously cast
> here didn't always worked since in some cases field_type
> and MEM_type of the mem not always were compatible. I think
> this was fixed in previous commits. All tests passes.
> 
> However, there is another possiblity: it may be that I
> already removed tests that fail on convertion but worked
> on check. I do not think that it is the case though.

Свят-свят. Давай подробнее. То что все тесты (которые перекромсали
десять раз) ходят - не особо важно. Катить в релиз что-то над чем
есть сомнения - не лучшая идея.
 
> This also can be fully fixed in #4906 issue.

Как?! Тот тикет про рефакторинг!

> > > +		if (mem_check_type(mem, type) == 0)
> > > +			continue;
> > > +		if (sql_type_is_numeric(type) &&
> > > +		    (mem->flags & (MEM_Real | MEM_Int | MEM_UInt)) != 0)
> > > +			continue;
> > > +		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > > +			field_type_strs[type], mem_type_to_str(mem));
> > > +		goto abort_due_to_error;
> > > +	}
> > >  #ifdef SQL_DEBUG
> > >  	{ int i; for(i=0; i<r.nField; i++) assert(memIsValid(&r.aMem[i])); }
> > >  #endif
> > > diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> > > index 6d8768865..1d7c76670 100644
> > > --- a/src/box/sql/wherecode.c
> > > +++ b/src/box/sql/wherecode.c
> > > @@ -335,72 +335,6 @@ disableTerm(WhereLevel * pLevel, WhereTerm * pTerm)
> > >  	}
> > >  }
> > >  
> > > -/**
> > > - * Code an OP_ApplyType opcode to apply the column type string
> > > - * @types to the n registers starting at @base.
> > > - *
> > > - * As an optimization, SCALAR entries (which are no-ops) at the
> > > - * beginning and end of @types are ignored.  If all entries in
> > > - * @types are SCALAR, then no code gets generated.
> > > - *
> > > - * This routine makes its own copy of @types so that the caller is
> > > - * free to modify @types after this routine returns.
> > > - */
> > > -static void
> > > -emit_apply_type(Parse *pParse, int base, int n, enum field_type *types)
> > 
> > Ты в следующем коммите как раз делаешь клин-ап. Давай эту функцию
> > там и удалим (как и остальные).
> > 
> It is a bit troublesome since the functions I remove here
> are 'static'. I think, that instead of removing 'static'
> keyword here and removing the function in the next commit
> it is better to just remove these runctions here.
> 
> I can change it if you think that removing in two commits
> is better.

> 
> commit f52e6669146e7a06b78d167abf369d67d9327789
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Thu Jun 25 12:39:19 2020 +0300
> 
>     sql: remove implicit cast for comparison
>     
>     This patch removes implicit cast for comparison.
>     
>     Closes #4230
>     
>     @TarantoolBot document
>     Title: remove implicit cast for comparison
>     
>     After this patch-set, there will be no implicit casts for
>     comparison. This means that the values ​​of the field types STRING,
>     BOOLEAN and VARBINARY can be compared with the values ​​of the same

Непечатные символы в тексте, проверь.

>     field type. Any numerical value can be compared with any other
>     numerical value.
>     
>     Example:
>     
>     For comparison:
>     ```
>     tarantool> box.execute([[SELECT '1' > 0;]])
>     ---
>     - null
>     - "Type mismatch: can not convert '1' (type: text) to numeric"
>     ...
>     
>     tarantool> box.execute([[SELECT true > X'33';]])
>     ---
>     - null
>     - 'Type mismatch: can not convert ''TRUE'' (type: boolean) to varbinary'
>     ...
>     
>     tarantool> box.execute([[SELECT 1.23 > 123;]])
>     ---
>     - metadata:
>       - name: 1.23 > 123
>         type: boolean
>       rows:
>       - [false]
>     ...
>     ```
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index ce9edaad9..0d3da49d3 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -561,6 +561,50 @@ mem_convert_to_numeric(struct Mem *mem, enum field_type type, bool is_precise)
>  	return mem_convert_to_integer(mem, is_precise);
>  }
>  
> +/**
> + * Check that MEM types of mems are compatible with the
> + * corresponding types from key_def.

Так compatible или comparable?

> + * If mem and type are comparable but incompatible according to
> + * field_mp_plain_type_is_compatible() and the comparison
> + * operation is EQ, we can say that the result of the comparison
> + * will be FALSE. This allows us to compare DOUBLE and INTEGER

Почему false? Не очевидно.

> + * values.
> + *
> + * @param mems The first mem of the array.
> + * @param def key_def that contains types to check.
> + * @param size Size of the array.
> + * @param is_op_eq TRUE is comparison operator is EQ.

-> if. Как на результат совместимости типов влияет оператор сравнения?

> + * @param[out] is_not_converted TRUE if operation is EQ and mem
> +                                and type are comparable but not
> +                                compatible. FALSE otherwise.

Ты просто перечислил условия, при котором выставляешь этот флаг.
Какой смысл в этом комментарии?

> + * @retval TRUE if the MEM types and types from key_def are
> + *         comparable, FALSE otherwise.

Ты про диаг забыл...

> + */
> +static bool
> +mem_are_types_comparable(struct Mem *mems, struct key_def *def, uint32_t size,
> +			 bool is_op_eq, bool *is_not_converted) {

Извини, но в этой функции плохо просто все. Давай по порядку:

- название: почему mem_are? Откуда types, если там кей деф. Сайз чего?
Что значит is_not_converted, если ты просто делаешь проверку на
совместимость?

- предполагается, что функция что-то проверяет и возвращает как
следствие результат проверки (_are_). Но почему-то она еще и
диаг выставляет. Не надо так.

> +	assert(!is_op_eq || !*is_not_converted);

- зачем тебе два флага, которые друг на друга как-то нетривиально
завязаны, причем в половине вызовов (1 из 2) они вообще не использу-
ются?

> +	for (uint32_t i = 0; i < size; ++i) {

- где ассерт i < def->part_count ?

> +		enum field_type type = def->parts[i].type;
> +		struct Mem *mem = &mems[i];
> +		if (mem_is_type_compatible(mem, type))
> +			continue;
> +		if (mem_convert_to_numeric(mem, type, true) == 0)
> +			continue;
> +		if (sql_type_is_numeric(type) &&
> +		    (mem->flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
> +			if (is_op_eq)
> +				*is_not_converted = true;
> +			continue;
> +		}

- тут вообще не понятно что происходит - тройной if с выставлением
выходного параметра. Я честно говорю, что я не понимаю что тут происходит.
sql_type_is_numeric() уже вызывается в mem_convert_to_numeric()...
Давай на днях вместе сядем и подумаем как это все исправить.

> +		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> +			field_type_strs[type], mem_type_to_str(mem));
> +		return false;
> +	}
> +	return true;
> +}
> +
> @@ -3623,7 +3645,20 @@ skip_truncate:
>  	assert(oc!=OP_SeekGE || r.default_rc==+1);
>  	assert(oc!=OP_SeekLT || r.default_rc==+1);
>  
> +	/*
> +	 * Make sure that the types of all the fields in the tuple
> +	 * that will be used in the iterator match the field types
> +	 * of the space.
> +	 */
>  	r.aMem = &aMem[pOp->p3];
> +	bool is_not_found = false;

Не очень логично, что функция проверяющая на совместимость типов, еще
принимает флаг is_not_found..?? Что она ищет?

> +	if (!mem_are_types_comparable(r.aMem, r.key_def, r.nField, eqOnly == 1,
> +				      &is_not_found))
> +		goto abort_due_to_error;
> +	if (is_not_found) {
> +		res = 1;
> +		goto seek_not_found;
> +	}
>  #ifdef SQL_DEBUG
>  	{ int i; for(i=0; i<r.nField; i++) assert(memIsValid(&r.aMem[i])); }
>  #endif
> @@ -4750,7 +4785,16 @@ case OP_IdxGE:  {       /* jump */
>  		assert(pOp->opcode==OP_IdxGE || pOp->opcode==OP_IdxLT);
>  		r.default_rc = 0;
>  	}
> +
> +	/*
> +	 * Make sure that the types of mems are comparable with
> +	 * the field types of the space.
> +	 */
>  	r.aMem = &aMem[pOp->p3];
> +	bool unused = false;
> +	if (!mem_are_types_comparable(r.aMem, r.key_def, r.nField, false,
> +				      &unused))
> +		goto abort_due_to_error;
>  #ifdef SQL_DEBUG
>  	{ int i; for(i=0; i<r.nField; i++) assert(memIsValid(&r.aMem[i])); }
>  #endif