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 3EDDA257B8 for ; Tue, 22 Jan 2019 10:41:46 -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 RRirIQijtnzV for ; Tue, 22 Jan 2019 10:41:46 -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 CCCDF2572E for ; Tue, 22 Jan 2019 10:41:45 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 5/8] sql: replace field type with affinity for VDBE runtime References: <6e7623ee-fdcc-616f-6a99-20aff0e97f43@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Tue, 22 Jan 2019 18:41:32 +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: "n.pettik" , tarantool-patches@freelists.org Thanks for the fixes! >> >>> /* 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? > > Both. p4type > 0 means that p4type bytes are copied to freshly allocated memory > and P4 set to DYNAMIC. sqlite3DbStrNDup() in vdbeChangeP4Full() reserves > one more byte for NULL termination. I see now, that OP_MakeRecord uses only p4.z and does not touch p4type, so it makes no sense to pass here length of the field_type array. Please, use only P4_STATIC and P4_DYNAMIC. At this moment this opcode is mess: somewhere 0 is passed, somewhere P4_DYNAMIC, in other places length of a field_type array. >>> + 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. > > Am I missing smth? > sqlite3Stat4ValueFromExpr -> stat4ValueFromExpr -> sqlite3ValueApplyAffinity -> mem_apply_type Naming confuses. All this functions except mem_apply_type take "u8 affinity", not "enum field_type type". Please, rename parameters and functions. > Author: Nikita Pettik > Date: Fri Dec 21 13:23:07 2018 +0200 > > sql: replace affinity with field type for VDBE runtime > > 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 > See below 6 comments. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index e51e2db2a..514d0ca9d 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index f9c42fdec..ca6c49373 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -592,13 +592,13 @@ sql_generate_index_key(struct Parse *parse, struct index *index, int cursor, > * is an integer, then it might be stored in the > * table as an integer (using a compact > * representation) then converted to REAL by an > - * OP_RealAffinity opcode. But we are getting > + * OP_Realify opcode. But we are getting 1. OP_RealAffinity still is mentioned in sqliteInt.h in a comment. > * ready to store this value back into an index, > * where it should be converted by to INTEGER > - * again. So omit the OP_RealAffinity opcode if > + * again. So omit the OP_Realify opcode if > * it is present > */ > - sqlite3VdbeDeletePriorOpcode(v, OP_RealAffinity); > + sqlite3VdbeDeletePriorOpcode(v, OP_Realify); > } > if (reg_out != 0) > sqlite3VdbeAddOp3(v, OP_MakeRecord, reg_base, col_cnt, reg_out); > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 0a80ca622..ca39faf51 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -3172,7 +3177,10 @@ 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); > + enum field_type *types = sql_affinity_str_to_field_type_str(zAff); > + types[nVector] = field_type_MAX; > + sqlite3VdbeAddOp4(v, OP_ApplyType, rLhs, nVector, 0, (char *)types, > + P4_DYNAMIC); 2. Why do you need types[nVector] = field_type_MAX? As I see, before your patch there was no additional zero termination. > 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/update.c b/src/box/sql/update.c > index 0e2d0fde8..fd74817ea 100644 > --- a/src/box/sql/update.c > +++ b/src/box/sql/update.c > @@ -274,11 +274,12 @@ sqlite3Update(Parse * pParse, /* The parser context */ > nKey = pk_part_count; > regKey = iPk; > } else { > - const char *zAff = is_view ? 0 : > - sql_space_index_affinity_str(pParse->db, def, > - pPk->def); > + enum field_type *types = is_view ? NULL : > + sql_index_type_str(pParse->db, > + pPk->def); > sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, pk_part_count, > - regKey, zAff, pk_part_count); > + regKey, (char *) types, > + is_view ? 0 : P4_DYNAMIC); 3. I guess, here and in other places, where type str is either 0 or not 0, you can always set P4_DYNAMIC, it looks a bit simpler and works as well. "is_view ? 0 : P4_DYNAMIC" -> "P4_DYNAMIC". > /* > * Set flag to save memory allocating one by > * malloc. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 4345af24e..61d73b676 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: 4. Not sure, if determine is an adjective or a noun. It is always a verb. So it should be 'is determine*D*', shouldn't it? > * > - * 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 flag is set to BLOB. > + * > + * @param record The value to apply type to. > + * @param type_t The type to be applied. 5. type_t -> f_type. By the way, why f_type and not just type? > */ > static int > -mem_apply_affinity(struct Mem *record, enum affinity_type affinity) > +mem_apply_type(struct Mem *record, enum field_type f_type) > diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c > index b124a1d53..efbc91cf8 100644 > --- a/src/box/sql/wherecode.c > +++ b/src/box/sql/wherecode.c > @@ -396,9 +396,12 @@ codeApplyAffinity(Parse * pParse, int base, int n, char *zAff) > n--; > } > > - /* Code the OP_Affinity opcode if there is anything left to do. */ > if (n > 0) { > - sqlite3VdbeAddOp4(v, OP_Affinity, base, n, 0, zAff, n); > + enum field_type *types = > + sql_affinity_str_to_field_type_str(zAff); > + types[n] = field_type_MAX; 6. The same as 2. > + sqlite3VdbeAddOp4(v, OP_ApplyType, base, n, 0, (char *)types, > + P4_DYNAMIC); > sqlite3ExprCacheAffinityChange(pParse, base, n); > } > }