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 E10E31FFDA for ; Sat, 29 Dec 2018 12:42:30 -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 IEDup9MaSEXW for ; Sat, 29 Dec 2018 12:42:30 -0500 (EST) 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 6DA7621851 for ; Sat, 29 Dec 2018 12:42:30 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 5/8] sql: replace field type with affinity for VDBE runtime References: From: Vladislav Shpilevoy Message-ID: <6e7623ee-fdcc-616f-6a99-20aff0e97f43@tarantool.org> Date: Sat, 29 Dec 2018 20:42:28 +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 8 comments below. > sql: replace field type with affinity for VDBE runtime 1. Maybe vice versa? Replace affinity with field type? On 28/12/2018 12:34, Nikita Pettik wrote: > This stage of affinity removal requires introducing of auxiliary > intermediate function to convert array of affinity values to field type > values. The rest of job done in this commit is a straightforward > refactoring. > > Part of #3698 > --- > src/box/sql/analyze.c | 5 +- > src/box/sql/build.c | 13 +++++ > src/box/sql/delete.c | 16 +++---- > src/box/sql/expr.c | 21 ++++---- > src/box/sql/fkey.c | 3 +- > src/box/sql/insert.c | 31 ++++++++---- > src/box/sql/select.c | 17 +++++-- > src/box/sql/sqliteInt.h | 13 +++-- > src/box/sql/update.c | 17 ++++--- > src/box/sql/vdbe.c | 92 ++++++++++++++++-------------------- > src/box/sql/vdbeInt.h | 2 +- > src/box/sql/vdbemem.c | 18 +++---- > src/box/sql/where.c | 2 +- > src/box/sql/wherecode.c | 6 +-- > test/sql-tap/cast.test.lua | 10 ++-- > test/sql-tap/tkt-80e031a00f.test.lua | 8 ++-- > 16 files changed, 156 insertions(+), 118 deletions(-) > > diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c > index 51c63fa7a..a04dc8681 100644 > --- a/src/box/sql/analyze.c > +++ b/src/box/sql/analyze.c > @@ -993,9 +993,10 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space) > sqlite3VdbeAddOp2(v, OP_Next, idx_cursor, next_row_addr); > /* Add the entry to the stat1 table. */ > callStatGet(v, stat4_reg, STAT_GET_STAT1, stat1_reg); > - assert("BBB"[0] == AFFINITY_TEXT); > + char types[3] = { FIELD_TYPE_STRING, FIELD_TYPE_STRING, > + FIELD_TYPE_STRING }; 2. How about explicit type? Not char[], but enum field_type[] ? It will look much more readable and convenient, IMO. Here and in other places. 'type_str', used now in all places instead of affinity str, looks crutchy. > sqlite3VdbeAddOp4(v, OP_MakeRecord, tab_name_reg, 3, tmp_reg, > - "BBB", 0); > + types, 3); > sqlite3VdbeAddOp4(v, OP_IdxInsert, tmp_reg, 0, 0, > (char *)stat1, P4_SPACEPTR); > /* Add the entries to the stat4 table. */ > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index e51e2db2a..b3f98c317 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -520,6 +520,19 @@ sql_field_type_to_affinity(enum field_type field_type) > } > } > > +char * 2. So not char *, but enum field_type *. Ok? > +sql_affinity_str_to_field_type_str(const char *affinity_str) > +{ > + if (affinity_str == NULL) > + return NULL; > + size_t len = strlen(affinity_str) + 1; > + char *type_str = (char *) sqlite3DbMallocRaw(sql_get(), len); > + for (uint32_t i = 0; i < len - 1; ++i) > + type_str[i] = sql_affinity_to_field_type(affinity_str[i]); > + type_str[len - 1] = '\0'; 2. Instead of 0 you can use field_type_MAX terminator, if we will move to enum field_type[]. > + return type_str; > +} > + > /* > * Add a new column to the table currently being constructed. > * > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index f9c42fdec..7b0d6b2fd 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -332,12 +332,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > */ > key_len = 0; > struct index *pk = space_index(space, 0); > - const char *zAff = is_view ? NULL : > - sql_space_index_affinity_str(parse->db, > - space->def, > - pk->def); > + char *type_str = is_view ? NULL : > + sql_index_type_str(parse->db, > + pk->def); > sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len, > - reg_key, zAff, pk_len); > + reg_key, type_str, is_view ? 0 : > + P4_DYNAMIC); 3. How did it work before your patch? Looks like it was a leak. Before the patch, pk_len was passed instead of STATIC/DYNAMIC. > /* Set flag to save memory allocating one > * by malloc. > */ > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 917e6e30b..c823c5a06 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -2858,8 +2858,11 @@ sqlite3CodeSubselect(Parse * pParse, /* Parsing context */ > jmpIfDynamic = -1; > } > r3 = sqlite3ExprCodeTarget(pParse, pE2, r1); > + char type = > + sql_affinity_to_field_type(affinity); > sqlite3VdbeAddOp4(v, OP_MakeRecord, r3, > - 1, r2, &affinity, 1); > + 1, r2, &type, > + 1); 4. I do not understand. Is p4type of sqlite3VdbeAddOp4 of OP_MakeRecord a length of an affinity string, or a type of its allocation? Or both? Also, I wonder how does this code works: if (zAffinity) { pRec = pData0; do{ mem_apply_type(pRec++, *(zAffinity++)); }while( zAffinity[0]); } in OP_MakeRecord. It assumes, that zAffinity is a null-terminated string, but in the code above you pass one char, without zero-termination. > sqlite3ExprCacheAffinityChange(pParse, > r3, 1); > sqlite3VdbeAddOp2(v, OP_IdxInsert, r2, > @@ -3172,7 +3175,8 @@ sqlite3ExprCodeIN(Parse * pParse, /* Parsing and code generating context */ > * of the RHS using the LHS as a probe. If found, the result is > * true. > */ > - sqlite3VdbeAddOp4(v, OP_Affinity, rLhs, nVector, 0, zAff, nVector); > + char *type_str = sql_affinity_str_to_field_type_str(zAff); > + sqlite3VdbeAddOp4(v, OP_ApplyType, rLhs, nVector, 0, type_str, nVector); 5. type_str is dynamically allocated, nVector (== p4type) is > 0. But freeP4 function in vdbeaux.c does not know how to free p4type > 0. So it is definitely a leak. Please, validate all places where dynamic type string is created. > if (destIfFalse == destIfNull) { > /* Combine Step 3 and Step 5 into a single opcode */ > sqlite3VdbeAddOp4Int(v, OP_NotFound, pExpr->iTable, > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 4345af24e..369fb4b79 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -306,32 +306,35 @@ applyNumericAffinity(Mem *pRec, int bTryForInt) > } > > /** > - * Processing is determine by the affinity parameter: > + * Processing is determine by the field type parameter: > * > - * AFFINITY_INTEGER: > - * AFFINITY_REAL: > - * Try to convert mem to an integer representation or a > - * floating-point representation if an integer representation > - * is not possible. Note that the integer representation is > - * always preferred, even if the affinity is REAL, because > - * an integer representation is more space efficient on disk. > + * INTEGER: > + * If memory holds floating point value and it can be > + * converted without loss (2.0 - > 2), it's type is > + * changed to INT. Otherwise, simply return success status. > * > - * AFFINITY_TEXT: > - * Convert mem to a text representation. > + * NUMBER: > + * If memory holds INT or floating point value, > + * no actions take place. > * > - * AFFINITY_BLOB: > - * No-op. mem is unchanged. > + * STRING: > + * Convert mem to a string representation. > * > - * @param record The value to apply affinity to. > - * @param affinity The affinity to be applied. > + * SCALAR: > + * Mem is unchanged, but flat is set to BLOB. 6. flat -> flag? > + * > + * @param record The value to apply type to. > + * @param type_t The type to be applied. > */ > static int > -mem_apply_affinity(struct Mem *record, enum affinity_type affinity) > +mem_apply_type(struct Mem *record, enum field_type f_type) > { > if ((record->flags & MEM_Null) != 0) > return 0; > - switch (affinity) { > - case AFFINITY_INTEGER: > + assert(f_type < field_type_MAX); 7. Double white-space. > + switch (f_type) { > + case FIELD_TYPE_INTEGER: > + case FIELD_TYPE_UNSIGNED: > if ((record->flags & MEM_Int) == MEM_Int) > return 0; > if ((record->flags & MEM_Real) == MEM_Real) { > diff --git a/src/box/sql/where.c b/src/box/sql/where.c > index 571b5af78..539296079 100644 > --- a/src/box/sql/where.c > +++ b/src/box/sql/where.c > @@ -1200,7 +1200,7 @@ whereRangeSkipScanEst(Parse * pParse, /* Parsing & code generating context */ > int nLower = -1; > int nUpper = index->def->opts.stat->sample_count + 1; > int rc = SQLITE_OK; > - u8 aff = sql_space_index_part_affinity(space->def, p, nEq); > + u8 aff = p->key_def->parts[nEq].type; 8. Why? Below in this function aff is used as affinity, not type. > > sqlite3_value *p1 = 0; /* Value extracted from pLower */ > sqlite3_value *p2 = 0; /* Value extracted from pUpper */