[tarantool-patches] Re: [PATCH 2/3] sql: remove redundant type derivation from QP

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index 6f72506ad..977c0fced 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -769,16 +769,6 @@ codeAllEqualityTerms(Parse * pParse,	/* Parsing context */
>  						  pLevel->addrBrk);
>  				VdbeCoverage(v);
>  			}
> -			if (type != NULL) {
> -				enum field_type rhs_type =
> -					sql_expr_type(pRight);
> -				if (sql_type_result(rhs_type, type[j]) ==
> -				    FIELD_TYPE_SCALAR) {
> -					type[j] = FIELD_TYPE_SCALAR;
> -				}
> -				if (sql_expr_needs_no_type_change(pRight, type[j]))
> -					type[j] = FIELD_TYPE_SCALAR;
> -			}
>  		}

There is a comment on that function:

>  * Before returning, @types is set to point to a buffer containing a
>  * copy of the column types array of the index allocated using
>  * sqlDbMalloc(). Except, entries in the copy of the string associated
>  * with equality constraints that use SCALAR type are set to
>  * SCALAR. This is to deal with SQL such as the following:
>  *
>  *   CREATE TABLE t1(a TEXT PRIMARY KEY, b BLOB);
>  *   SELECT ... FROM t1 AS t2, t1 WHERE t1.a = t2.b;
>  *
>  * In the example above, the index on t1(a) has STRING type. But since
>  * the right hand side of the equality constraint (t2.b) has SCALAR type,
>  * no conversion should be attempted before using a t2.b value as part of
>  * a key to search the index. Hence the first byte in the returned type
>  * string in this example would be set to SCALAR.

Looks outdated, especially after your change. Now we do not
convert to scalars. Even before your patch it was outdated, as I
understand. Lets fix it alongside, since now it makes no sense at
all.

Also I need you comment on that code, which is above the hunk
you deleted:

> 		if (pTerm->eOperator & WO_IN) {
> 			if (pTerm->pExpr->flags & EP_xIsSelect) {
> 				/* No type ever needs to be (or should be) applied to a value
> 				 * from the RHS of an "? IN (SELECT ...)" expression. The
> 				 * sqlFindInIndex() routine has already ensured that the
> 				 * type of the comparison has been applied to the value.
> 				 */
> 				if (type != NULL)
> 					type[j] = FIELD_TYPE_SCALAR;
> 			}

Looks, like this code still thinks, that we put into IN operator
values of any mixed types, but I thought, that we want to forbid
that. That IN should have only values of the same type. Isn't it
a bug?