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 C4B152659E for ; Wed, 30 Jan 2019 08:04:31 -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 3QlJwxPkZ7Kc for ; Wed, 30 Jan 2019 08:04:31 -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 7F72F25E94 for ; Wed, 30 Jan 2019 08:04:31 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 7/8] sql: clean-up affinity from SQL source code References: <541AF1EA-6688-4B4F-92CB-2AA15765A62E@tarantool.org> <2cd04a2d-6cd8-9e0b-7d51-d9431b6c5d82@tarantool.org> <33150EBA-AB14-4089-8399-0FBD07C71BD8@tarantool.org> From: Vladislav Shpilevoy Message-ID: <1487a209-af7b-5d1f-6fff-fc589782d571@tarantool.org> Date: Wed, 30 Jan 2019 16:04:29 +0300 MIME-Version: 1.0 In-Reply-To: <33150EBA-AB14-4089-8399-0FBD07C71BD8@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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! See comments below. >>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >>> index 61d73b676..c3f596d4f 100644 >>> --- a/src/box/sql/vdbe.c >>> +++ b/src/box/sql/vdbe.c >>> @@ -2163,9 +2151,16 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */ >>> break; >>> } >>> } else { >>> - /* Neither operand is NULL. Do a comparison. */ >>> - affinity = pOp->p5 & AFFINITY_MASK; >>> - if (affinity>=AFFINITY_INTEGER) { >>> + /* >>> + * Neither operand is NULL. Do a comparison. >>> + * 15 is 1111 in a binary form. >>> + * Since all existing types can be fitted in 4 bits >>> + * (field_type_MAX == 10), it is enough to 'and' >>> + * p5 with this constant. Node that all other flags >>> + * that can be stored in p5 are >= 16. >>> + */ >>> + affinity = pOp->p5 & 15; >> >> 4. Honestly, I did not expect it from you. Please, never use such >> constants and assumptions about them in code. field_type_MAX = 10 can >> be changed any day (for example, when we introduce decimal as a native >> Tarantool data type, or decide to add datetime). In fact, it is already >> not 10 - it is 9. > > Oh, cmon. This place will explode only if number of types exceed 15. > Not so soon, I guess (when was the last time we introduced new type?). > So it won’t be my problem by the time it occurs. > >> I propose you to either come up with a better solution or to use one of >> my solutions: >> >> * return AFFINITY_MASK as P5_FIELD_TYPE_MASK, and add a static assertion >> that P5_FIELD_TYPE_MASK < (1 << bit_count(field_type)). >> >> Or write in field_def.h something like this: >> >> static_assert(bit_count(field_type) <= 4, >> "size of enum field_type is used in "\ >> "VdbeOp.p5 to fit it into 4 bits”); > > To be honest I don’t understand these bit assertions. > Why can’t we simply check that field_type_MAX <= 15? > > diff --git a/src/box/field_def.c b/src/box/field_def.c > index b34d2ccd9..1a8074650 100644 > --- a/src/box/field_def.c > +++ b/src/box/field_def.c > @@ -33,6 +33,13 @@ > #include "trivia/util.h" > #include "key_def.h" > > +/** > + * For detailed explanation see context of OP_Eq, OP_Lt etc > + * opcodes in vdbe.c. > + */ > +static_assert(field_type_MAX <= 15, "values of enum field_type "\ > + "should fit into 4 bits of VdbeOp.p5"); > + > > Also, as you may notice, I placed it to field_def.c > Seems that field_def.h is used to generate module.h, > which in turn doesn’t tolerate this kind of assert. field_def.h is not copied to module.h entirely. Only the code between /** \cond public */ and /** \endcond public */. Please, put this assertion below /** \endcond public */. > >> To get bit_count during compilation you can specify it explicitly right >> after field_type_MAX in enum field_type so as to change it always >> together with field_type_MAX. >> > diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c > index 53445faf5..1f28893f6 100644 > --- a/src/box/sql/wherecode.c > +++ b/src/box/sql/wherecode.c >>> @@ -419,16 +419,15 @@ codeApplyAffinity(Parse * pParse, int base, int n, char *zAff) >>> static void >>> updateRangeAffinityStr(Expr * pRight, /* RHS of comparison */ >>> int n, /* Number of vector elements in comparison */ >>> - char *zAff) /* Affinity string to modify */ >>> + enum field_type *zAff) /* Affinity string to modify */ >> >> 6. The same. In many other places too. Where you changed type and >> usages of a variable, but kept its name. > > I did it on purpose - to reduce diff, so that separate functional > changes and non-functional such as renaming. Now fixed. Small diff is good, but lets do not overemphasize. This patch states that affinity is removed, and not only as a functionality, but from the source code, but before your fixes it actually was not. It complicated reading and understanding of the code. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 80d2fd0aa..358b69754 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -2164,17 +2151,24 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */ > break; > } > } else { > - /* Neither operand is NULL. Do a comparison. */ > - affinity = pOp->p5 & AFFINITY_MASK; > - if (affinity>=AFFINITY_INTEGER) { > + /* > + * Neither operand is NULL. Do a comparison. > + * 15 is 1111 in a binary form. > + * Since all existing types can be fitted in 4 bits > + * (field_type_MAX == 10), it is enough to 'and' > + * p5 with this constant. Node that all other flags > + * that can be stored in p5 are >= 16. Firstly, as I said in the previous review, field_type_MAX == 9. Secondly, please, do not use a constant. All of them (15, 111, 10, 16). Instead of this huge comment full of constants just create a enum value with an appropriate name, like FIELD_TYPE_MASK = 15, it is even ok to put this name into field_def.h. Thirdly, not all p5 flags are >= 16. But for these concrete opcodes it is so, luckily. At least we have OPFLAG_NCHANGE, OPFLAG_EPHEM, OPFLAG_SEEKEQ etc - they all are <= 15. > + */ > + enum field_type type = pOp->p5 & 15; > + if (sql_type_is_numeric(type)) { > if ((flags1 | flags3)&MEM_Str) { > if ((flags1 & (MEM_Int|MEM_Real|MEM_Str))==MEM_Str) { > - applyNumericAffinity(pIn1,0); > + mem_apply_numeric_type(pIn1, 0); > testcase( flags3!=pIn3->flags); /* Possible if pIn1==pIn3 */ > flags3 = pIn3->flags; > } > if ((flags3 & (MEM_Int|MEM_Real|MEM_Str))==MEM_Str) { > - if (applyNumericAffinity(pIn3,0) != 0) { > + if (mem_apply_numeric_type(pIn3, 0) != 0) { > diag_set(ClientError, > ER_SQL_TYPE_MISMATCH, > sqlite3_value_text(pIn3),