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 988A220D6B for ; Sat, 29 Dec 2018 12:42:27 -0500 (EST) 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 sURyqQpZQo4x for ; Sat, 29 Dec 2018 12:42:27 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 56D7D20B16 for ; Sat, 29 Dec 2018 12:42:27 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 7/8] sql: clean-up affinity from SQL source code References: From: Vladislav Shpilevoy Message-ID: Date: Sat, 29 Dec 2018 20:42:24 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed 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 Thanks for the patch! See 5 comments below. On 28/12/2018 12:34, Nikita Pettik wrote: > Replace remains of affinity usage in SQL parser, query optimizer and > VDBE. Don't add affinity to field definition when table is encoded into > msgpack. Remove field type <-> affinity converters, since now we can > operate directly on field type. > > Part of #3698 > --- > src/box/sql.c | 6 +----- > src/box/sql/build.c | 52 ------------------------------------------------ > src/box/sql/expr.c | 11 +++++----- > src/box/sql/insert.c | 39 ++++++------------------------------ > src/box/sql/select.c | 17 +++++----------- > src/box/sql/sqliteInt.h | 53 ------------------------------------------------- > src/box/sql/vdbe.c | 4 ++-- > src/box/sql/where.c | 10 ---------- > src/box/sql/wherecode.c | 29 ++++++++++----------------- > src/box/sql/whereexpr.c | 3 ++- > test/sql/types.result | 15 +++++++------- > test/sql/upgrade.result | 6 +++--- > 12 files changed, 42 insertions(+), 203 deletions(-) > > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 32606dac3..22b64b526 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -311,8 +311,7 @@ binaryCompareP5(Expr * pExpr1, Expr * pExpr2, int jumpIfNull) > { > enum field_type lhs = sql_expr_type(pExpr2); > enum field_type rhs = sql_expr_type(pExpr1); > - u8 type_mask = sql_field_type_to_affinity(sql_type_result(rhs, lhs)) | > - (u8) jumpIfNull; > + u8 type_mask = sql_type_result(rhs, lhs) | (u8) jumpIfNull; 1. Are you sure that we can | jumpIfNull with enum field_type? Look at this code: #define SQLITE_KEEPNULL 0x08 /* Used by vector == or <> */ #define SQLITE_JUMPIFNULL 0x10 /* jumps if either operand is NULL */ #define SQLITE_STOREP2 0x20 /* Store result in reg[P2] rather than jump */ #define SQLITE_NULLEQ 0x80 /* NULL=NULL */ #define SQLITE_NOTNULL 0x90 /* Assert that operands are never NULL */ SQLite states that these values can be safely ORed with type, but SQLITE_KEEPNULL == FIELD_TYPE_MAP - mess. > return type_mask; > } > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index cc3e2f2fd..f3008094b 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -3067,10 +3061,9 @@ generateOutputSubroutine(struct Parse *parse, struct Select *p, > int r1; > testcase(in->nSdst > 1); > r1 = sqlite3GetTempReg(parse); > - const char *type_str = > - sql_affinity_str_to_field_type_str(dest->zAffSdst); > sqlite3VdbeAddOp4(v, OP_MakeRecord, in->iSdst, > - in->nSdst, r1, type_str, P4_DYNAMIC); > + in->nSdst, r1, dest->zAffSdst, > + in->nSdst); 2. As we learned from the previous patch, p4 here is not a length. It is a type of memory - static or dynamic. > sqlite3ExprCacheAffinityChange(parse, in->iSdst, > in->nSdst); > sqlite3VdbeAddOp2(v, OP_IdxInsert, r1, dest->reg_eph); > diff --git a/src/box/sql/where.c b/src/box/sql/where.c > index ac5390f92..88c45aaa3 100644 > --- a/src/box/sql/where.c > +++ b/src/box/sql/where.c > @@ -701,7 +701,6 @@ termCanDriveIndex(WhereTerm * pTerm, /* WHERE clause term to check */ > return 0; > if (pTerm->u.leftColumn < 0) > return 0; > - aff = pSrc->pTab->aCol[pTerm->u.leftColumn].affinity; > if (!sqlite3IndexAffinityOk(pTerm->pExpr, aff)) 3. This function does not exist since the previous commit. Looks like this code is not even compiled. > return 0; > return 1; 4. AFFINITY_MASK still exists. 5. wherecode.c and vdbeaux.c still use AFFINITY_BLOB in comments.