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 428F1252C6 for ; Tue, 22 Jan 2019 10:41:28 -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 B2i067pr7_ZZ for ; Tue, 22 Jan 2019 10:41:28 -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 71C5A2482B for ; Tue, 22 Jan 2019 10:41:27 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 6/8] sql: replace affinity with field type in struct Expr References: <406a1cab-d3cd-437b-668e-141697fbd637@tarantool.org> <8D9E26B4-FC3D-4EBD-9941-5570A5ECFA17@tarantool.org> From: Vladislav Shpilevoy Message-ID: <2837d5ad-fc59-8c3e-0fef-b82ea764f6ca@tarantool.org> Date: Tue, 22 Jan 2019 18:41:13 +0300 MIME-Version: 1.0 In-Reply-To: <8D9E26B4-FC3D-4EBD-9941-5570A5ECFA17@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! >>> -int >>> -sqlite3IndexAffinityOk(Expr * pExpr, char idx_affinity) >>> +enum field_type >>> +sql_index_type_is_ok(struct Expr *expr, enum field_type idx_type) >> >> 4. Strictly speaking, it is not idx_type. It is a type of one >> column. I think, it should be renamed to just type, or field_type, >> as well as in struct WhereScan. > > Yea, it’s fair. Renamed: > > -/* > - * pExpr is a comparison operator. Return the type affinity that should > - * be applied to both operands prior to doing the comparison. > +/** > + * pExpr is a comparison operator. Return the type affinity > + * that should be applied to both operands prior to doing > + * the comparison. > */ > static enum field_type > -comparisonAffinity(Expr * pExpr) > +expr_cmp_mutual_type(struct Expr *pExpr) > { > assert(pExpr->op == TK_EQ || pExpr->op == TK_IN || pExpr->op == TK_LT || > pExpr->op == TK_GT || pExpr->op == TK_GE || pExpr->op == TK_LE || > Are you sure? Looks like it is not renamed. Also, the comment was about sql_index_type_is_ok and its parameter idx_type, but you changed comparisonAffinity function. >>> + default: >>> + return 0; >>> } >>> } >>> @@ -2826,16 +2795,15 @@ sqlite3CodeSubselect(Parse * pParse, /* Parsing context */ >>> * that columns affinity when building index keys. If is not >>> * a column, use numeric affinity. >>> */ >>> - char affinity; /* Affinity of the LHS of the IN */ >>> int i; >>> ExprList *pList = pExpr->x.pList; >>> struct ExprList_item *pItem; >>> int r1, r2, r3; >>> - affinity = sqlite3ExprAffinity(pLeft); >>> - if (!affinity) { >>> - affinity = AFFINITY_BLOB; >>> - } >>> + enum field_type lhs_type = >>> + sql_expr_type(pLeft); >>> + if (lhs_type == FIELD_TYPE_ANY) >>> + lhs_type = FIELD_TYPE_SCALAR; >> >> 7. Why did not you move this conversion to sql_expr_type? I mean >> ANY -> SCALAR. > > Unfortunately, right now I can’t replace it with ANY. It is still used > when IN operator comes with one operand (see EP_Generic flag). > I have already sent patch which removes it: > https://github.com/tarantool/tarantool/tree/np/gh-3934-IN-operator-fix I've already acked the patch. Please, hurry up Kirill Y. to push it, rebase this branch, and replace ANY with SCALAR. > > After it hits 2.1, I will replace SCALAR with ANY. Vice versa? Not SCALAR with ANY, but ANY with SCALAR, I think. > >> ANY differs from SCALAR in only one thing - it is >> able to store MP_MAP and MP_ARRAY. So I am slightly bent upon >> frequency of ANY usage in SQL, wherein MAP/ARRAY does not exist. > > But still we can SELECT data from spaces created from Lua. > For instance, _fk_constraint features type ARRAY in its format, > so we can’t ignore this type even now (IMHO). Despite our ability to select complex types, in SQL they are all mere binary data with *sub*type msgpack, not array/map type. So in SQL we never truly work with anything but SCALAR. See 5 comments below. > -/* > - * Return TRUE if the given expression is a constant which would be > - * unchanged by OP_ApplyType with the type given in the second > - * argument. > - * > - * This routine is used to determine if the OP_ApplyType operation > - * can be omitted. When in doubt return FALSE. A false negative > - * is harmless. A false positive, however, can result in the wrong > - * answer. > - */ > int > -sqlite3ExprNeedsNoAffinityChange(const Expr * p, char aff) > +sql_expr_needs_no_type_change(const Expr *p, enum field_type type) 1. const Expr -> const struct Expr > -int sqlite3ExprNeedsNoAffinityChange(const Expr *, char); > + > +/** > + * Return TRUE if the given expression is a constant which would > + * be unchanged by OP_ApplyType with the type given in the second > + * argument. > + * > + * This routine is used to determine if the OP_ApplyType operation > + * can be omitted. When in doubt return FALSE. A false negative > + * is harmless. A false positive, however, can result in the wrong > + * answer. > + */ > +int > +sql_expr_needs_no_type_change(const Expr *epr, enum field_type type); 2. epr -> p, and the same as 1. > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c > index cd71641b0..d12a2a833 100644 > --- a/src/box/sql/vdbemem.c > +++ b/src/box/sql/vdbemem.c > @@ -1316,8 +1315,8 @@ valueFromExpr(sqlite3 * db, /* The database connection */ > sqlite3ValueSetStr(pVal, -1, zVal, SQLITE_DYNAMIC); > } > if ((op == TK_INTEGER || op == TK_FLOAT) > - && affinity == AFFINITY_BLOB) { > - sqlite3ValueApplyAffinity(pVal, AFFINITY_REAL); > + && affinity == FIELD_TYPE_SCALAR) { > + sqlite3ValueApplyAffinity(pVal, FIELD_TYPE_INTEGER); 3. Why before your patch it was REAL and now it is INTEGER? > } else { > sqlite3ValueApplyAffinity(pVal, affinity); > } > diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c > index efbc91cf8..33b860f36 100644 > --- a/src/box/sql/wherecode.c > +++ b/src/box/sql/wherecode.c > @@ -423,10 +423,11 @@ updateRangeAffinityStr(Expr * pRight, /* RHS of comparison */ > { > int i; > for (i = 0; i < n; i++) { > + enum field_type type = sql_affinity_to_field_type(zAff[i]); > Expr *p = sqlite3VectorFieldSubexpr(pRight, i); > - enum affinity_type aff = sqlite3ExprAffinity(p); > - if (sql_affinity_result(aff, zAff[i]) == AFFINITY_BLOB > - || sqlite3ExprNeedsNoAffinityChange(p, zAff[i])) { > + enum field_type expr_type = sql_expr_type(p); > + if (sql_type_result(expr_type, type) == FIELD_TYPE_SCALAR || > + sql_expr_needs_no_type_change(p, type)) { > zAff[i] = AFFINITY_BLOB; > } > } 4. Something is wrong with formatting. sql_expr_needs_no_type_change should be aligned under sql_type_result as a part of the condition. > @@ -1166,20 +1167,12 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t > } > struct index_def *idx_pk = space->index[0]->def; > int fieldno = idx_pk->key_def->parts[0].fieldno; > - char affinity = is_format_set ? > - space->def->fields[fieldno].affinity : > - AFFINITY_BLOB; > - if (affinity == AFFINITY_UNDEFINED) { > - if (idx_pk->key_def->part_count == 1 && > - space->def->fields[fieldno].type == > - FIELD_TYPE_INTEGER) > - affinity = AFFINITY_INTEGER; > - else > - affinity = AFFINITY_BLOB; > - } > + char fd_type = is_format_set ? > + space->def->fields[fieldno].type : > + FIELD_TYPE_ANY; > 5. Why char? And the alignment is slightly wrong. Non-first lines should be aligned under is_format_set.