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

[Nikita Pettik <korablev@tarantool.org>]

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.
 
> >> @@ -0,0 +1,281 @@
> >> +#!/usr/bin/env tarantool
> >> +test = require("sqltester")
> >> +test:plan(32)
> >> +
> >> +--
> >> +
> >> +test:execsql([[
> >> +    CREATE TABLE t1(x TEXT primary key);
> >> +    INSERT INTO t1 VALUES('1');
> >> +    CREATE TABLE t2(x INTEGER primary key);
> >> +    INSERT INTO t2 VALUES(1);
> >> +    CREATE TABLE t3(x DOUBLE primary key);
> >> +    INSERT INTO t3 VALUES(1.0);
> >
> > What about table with scalar/any fields?
> >
> I think this is too big a problem to solve it here.

Гораздо лучше потом клепать фоллоу-апы, точечно затыкая креши..
Я бы добавил хотя бы самых базовых примеров.

>  			i = pIn3->u.i;
> @@ -3617,7 +3595,38 @@ 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];
> +	for (int i = 0; i < r.nField; ++i) {
> +		enum field_type type = r.key_def->parts[i].type;
> +		struct Mem *mem = &r.aMem[i];
> +		if (mem_check_type(mem, type) == 0 ||
> +		    mem_convert_numeric(mem, type, true) == 0)
> +			continue;

Я бы все-таки разбил бы эту проверку на два условия:

if (...)
	continue
if (...)
	continue

А еще лучше вынести этот чанк в хелпер.

> +		/*
> +		 * If the number was not converted without loss,
> +		 * we will not find tuples using the EQ iterator.
> +		 */

Пожайлуста, не используй двойное отрицание - комментарий очень
сложно воспринимать. Давай это переформулируем, можно пример
добавить. Это довольно скользкое место.

> +		if ((mem->flags & MEM_Real) != 0 &&
> +		    (type == FIELD_TYPE_INTEGER ||
> +		     type == FIELD_TYPE_UNSIGNED)) {
> +			assert(eqOnly == 1);
> +			res = 1;
> +			goto seek_not_found;
> +		}
> +		if ((mem->flags & (MEM_Int | MEM_UInt)) != 0 &&
> +		    type == FIELD_TYPE_DOUBLE && eqOnly == 1) {
> +			res = 1;
> +			goto seek_not_found;
> +		}
> +		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
> @@ -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) вообще не нравятся.
Какие-то они запутанные. Давай подумаем как можно зарефакторить их.

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

Ты в следующем коммите как раз делаешь клин-ап. Давай эту функцию
там и удалим (как и остальные).

>  			nConstraint++;
>  		}
> -		sqlDbFree(db, start_types);
> -		sqlDbFree(db, end_types);
>  
>  		/* Top of the loop body */
>  		pLevel->p2 = sqlVdbeCurrentAddr(v);
> diff --git a/test/sql-tap/gh-4230-del-impl-cast-str-to-num.test.lua b/test/sql-tap/gh-4230-del-impl-cast-str-to-num.test.lua
> new file mode 100755
> index 000000000..b405a11b6
> --- /dev/null
> +++ b/test/sql-tap/gh-4230-del-impl-cast-str-to-num.test.lua

Когда кто-то ищет где посмотреть тесты связанные с типами,
первое что приходит в голову - грепнуть *types*. Этот тест
по сути ничего и не тестирует (уверен, эти пути уже покрыты
в других тестах), зато хорошо отображает работу системы типов.
Я бы просто подумал о других разработчиках, и все же замержил
его в какой-нибудь тест типа sql/types.test.lua.