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

[n.pettik]

> On 28 May 2019, at 00:49, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
> 
> 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.

Ok:

diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 977c0fced..d776c8ade 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -665,18 +665,8 @@ codeEqualityTerm(Parse * pParse,   /* The parsing context */
  *
  * 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.
+ * sqlDbMalloc(). This array is passed to OP_ApplyType to provide
+ * correct implicit conversions.
  */
 static int
 codeAllEqualityTerms(Parse * pParse,   /* Parsing context */

> 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?

Why do you think so? IN operator is decomposed into a series of
equality constraints, so it uses the same comparison rules as
other comparison operators. This code concerns pattern
? IN (SELECT …). In turn, in this situation index can be used only
if types of result columns are the same as ? column: it is verified
in sqlFindIndex:

/*
 * Index search is possible only if types
 * of columns match.
 */
if (idx_type != lhs_type)
       type_is_suitable = false;

Hence, we may skip implicit cast on these fields.