From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 15EDD2E3CE for ; Tue, 28 May 2019 15:58:33 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 7RsUmm51JJYL for ; Tue, 28 May 2019 15:58:33 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id C69F82CC40 for ; Tue, 28 May 2019 15:58:32 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH 2/3] sql: remove redundant type derivation from QP From: "n.pettik" In-Reply-To: <1184f246-6b89-eec7-94e7-7b87aabafcb9@tarantool.org> Date: Tue, 28 May 2019 22:58:29 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20bccdb8bed2bab9e9eaaca3781e3d0b5b780d10.1558700151.git.korablev@tarantool.org> <1184f246-6b89-eec7-94e7-7b87aabafcb9@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy > On 28 May 2019, at 00:49, Vladislav Shpilevoy = wrote: >=20 > Hi! Thanks for the patch! >=20 >> 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 !=3D NULL) { >> - enum field_type rhs_type =3D >> - sql_expr_type(pRight); >> - if (sql_type_result(rhs_type, type[j]) = =3D=3D >> - FIELD_TYPE_SCALAR) { >> - type[j] =3D FIELD_TYPE_SCALAR; >> - } >> - if = (sql_expr_needs_no_type_change(pRight, type[j])) >> - type[j] =3D FIELD_TYPE_SCALAR; >> - } >> } >=20 > There is a comment on that function: >=20 >> * 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 =3D 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. >=20 > 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 =3D 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: >=20 >> 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 !=3D NULL) >> type[j] =3D FIELD_TYPE_SCALAR; >> } >=20 > 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 =E2=80=A6). 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 !=3D lhs_type) type_is_suitable =3D false; Hence, we may skip implicit cast on these fields.