[tarantool-patches] Re: [PATCH 2/3] sql: remove redundant type derivation from QP
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue May 28 00:49:15 MSK 2019
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?
More information about the Tarantool-patches
mailing list