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 7A2E4265B4 for ; Wed, 30 Jan 2019 08:04:33 -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 M-fOvDga1-9r for ; Wed, 30 Jan 2019 08:04:33 -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 36A9125E94 for ; Wed, 30 Jan 2019 08:04:33 -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> <2837d5ad-fc59-8c3e-0fef-b82ea764f6ca@tarantool.org> <7FD8757A-713C-4887-9268-42849FFEE3A8@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 30 Jan 2019 16:04:31 +0300 MIME-Version: 1.0 In-Reply-To: <7FD8757A-713C-4887-9268-42849FFEE3A8@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! >>>> 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. > > I had to add some other fixes to make this happen: > > Changes to sql: replace affinity with field type for func: > > diff --git a/test-run b/test-run > index 02207efd2..4c3f11812 160000 > --- a/test-run > +++ b/test-run > @@ -1 +1 @@ > -Subproject commit 02207efd2ca44067b76c79bf012f142016d929ae > +Subproject commit 4c3f11812c0c17f08780f31b6a748eef1126b2e2 1. ??? > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index fdf00273a..e1a0dfa73 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -92,16 +111,14 @@ sql_expr_type(struct Expr *pExpr) > * return INTEGER. > */ > return FIELD_TYPE_INTEGER; > - } > - /* > - * In case of unary plus we shouldn't discard > - * type of operand (since plus always features > - * NUMERIC type). > - */ > - if (op == TK_UPLUS) { > + case TK_UMINUS: > + case TK_UPLUS: > + case TK_NO: > + case TK_BITNOT: > assert(pExpr->pRight == NULL); > - return pExpr->pLeft->type; > + return sql_expr_type(pExpr->pLeft); > } > + 2. Stray empty line. > return pExpr->type; >>>> 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. > > Wait, at the stage of query compiling we don’t operate on > any subtypes. We simply fetch space_def->fields->type, > which may turn out to be FIELD_TYPE_MAP/ARRAY or > whatever. Hmm, yes, you are right. We should consider array and map too. See 4 comments below. > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index d263c1bb5..e1a0dfa73 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -81,27 +57,46 @@ sqlite3ExprAffinity(Expr * pExpr) > case TK_SELECT: > assert(pExpr->flags & EP_xIsSelect); > el = pExpr->x.pSelect->pEList; > - return sqlite3ExprAffinity(el->a[0].pExpr); > + return sql_expr_type(el->a[0].pExpr); > case TK_CAST: > assert(!ExprHasProperty(pExpr, EP_IntValue)); > - return pExpr->affinity; > + return pExpr->type; > case TK_AGG_COLUMN: > case TK_COLUMN: > + case TK_TRIGGER: > assert(pExpr->iColumn >= 0); > - return sqlite3TableColumnAffinity(pExpr->space_def, > - pExpr->iColumn); > + return pExpr->space_def->fields[pExpr->iColumn].type; > case TK_SELECT_COLUMN: > assert(pExpr->pLeft->flags & EP_xIsSelect); > el = pExpr->pLeft->x.pSelect->pEList; > - return sqlite3ExprAffinity(el->a[pExpr->iColumn].pExpr); > + return sql_expr_type(el->a[pExpr->iColumn].pExpr); > case TK_PLUS: > case TK_MINUS: > case TK_STAR: > case TK_SLASH: > + case TK_REM: > + case TK_BITAND: > + case TK_BITOR: > + case TK_LSHIFT: > + case TK_RSHIFT: > assert(pExpr->pRight != NULL && pExpr->pLeft != NULL); > - enum affinity_type lhs_aff = sqlite3ExprAffinity(pExpr->pLeft); > - enum affinity_type rhs_aff = sqlite3ExprAffinity(pExpr->pRight); > - return sql_affinity_result(rhs_aff, lhs_aff); > + enum field_type lhs_type = sql_expr_type(pExpr->pLeft); > + enum field_type rhs_type = sql_expr_type(pExpr->pRight); > + return sql_type_result(rhs_type, lhs_type); > + case TK_CONCAT: > + return FIELD_TYPE_STRING; 1. Indentation. > + case TK_CASE: > + assert(pExpr->x.pList->nExpr >= 2); > + /* > + * CASE expression comes at least with one > + * WHEN and one THEN clauses. So, first > + * expression always represents WHEN > + * argument, and the second one - THEN. > + * > + * TODO: We must ensure that all THEN clauses > + * have arguments of the same type. > + */ > + return sql_expr_type(pExpr->x.pList->a[1].pExpr); > case TK_LT: > case TK_GT: > case TK_EQ: > @@ -252,74 +245,57 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id) > -/* > - * pExpr is a comparison expression, eg. '=', '<', IN(...) etc. > - * idx_affinity is the affinity of an indexed column. Return true > - * if the index with affinity idx_affinity may be used to implement > - * the comparison in pExpr. > +/** > + * @param expr is a comparison expression, eg. '=', '<', IN(...) etc. > + * @param field_type is the type of an indexed column. > + * @retval Return true if the index with @field_type may be used to > + * implement the comparison in expr. > */ > -int > -sqlite3IndexAffinityOk(Expr * pExpr, char idx_affinity) > +enum field_type > +sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type field_type) 2. Returns 'bool true', but the return value type is enum field_type? Please, change to bool. > { > - char aff = comparisonAffinity(pExpr); > - switch (aff) { > - case AFFINITY_BLOB: > + enum field_type type = expr_cmp_mutual_type(expr); > + switch (type) { > + case FIELD_TYPE_SCALAR: > return 1; 3. 1 -> true > - case AFFINITY_TEXT: > - return idx_affinity == AFFINITY_TEXT; > + case FIELD_TYPE_STRING: > + return field_type == FIELD_TYPE_STRING; > default: > - return sqlite3IsNumericAffinity(idx_affinity); > + return sql_type_is_numeric(field_type); > } > } > > @@ -2140,21 +2116,11 @@ sqlite3ExprCanBeNull(const Expr * p) > } > } > > -/* > - * 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 struct Expr *p, enum field_type type) 4. The same. Anyway you changed the function name, so without increasing the diff you can change its return value to bool, and the values below 0 -> false, 1 -> true. > { > u8 op; > - if (aff == AFFINITY_BLOB) > + if (type == FIELD_TYPE_SCALAR) > return 1; > while (p->op == TK_UPLUS || p->op == TK_UMINUS) { > p = p->pLeft;