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 965942BED9 for ; Mon, 27 May 2019 17:49:18 -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 5RbyXVDnhOHi for ; Mon, 27 May 2019 17:49:18 -0400 (EDT) Received: from smtp21.mail.ru (smtp21.mail.ru [94.100.179.250]) (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 56CC52BE85 for ; Mon, 27 May 2019 17:49:18 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/3] sql: remove redundant type derivation from QP References: <20bccdb8bed2bab9e9eaaca3781e3d0b5b780d10.1558700151.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <1184f246-6b89-eec7-94e7-7b87aabafcb9@tarantool.org> Date: Tue, 28 May 2019 00:49:15 +0300 MIME-Version: 1.0 In-Reply-To: <20bccdb8bed2bab9e9eaaca3781e3d0b5b780d10.1558700151.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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, Nikita Pettik 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?