From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: "n.pettik" <korablev@tarantool.org>, tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH 6/8] sql: replace affinity with field type in struct Expr Date: Wed, 30 Jan 2019 16:04:31 +0300 [thread overview] Message-ID: <aa4d3548-87bc-d738-8c65-84a51cacf6e7@tarantool.org> (raw) In-Reply-To: <7FD8757A-713C-4887-9268-42849FFEE3A8@tarantool.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;
next prev parent reply other threads:[~2019-01-30 13:04 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-12-28 9:34 [tarantool-patches] [PATCH 0/8] Eliminate affinity from source code Nikita Pettik 2018-12-28 9:34 ` [tarantool-patches] [PATCH 1/8] sql: remove SQLITE_ENABLE_UPDATE_DELETE_LIMIT define Nikita Pettik 2018-12-29 17:42 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-16 14:25 ` n.pettik 2018-12-28 9:34 ` [tarantool-patches] [PATCH 2/8] sql: use field type instead of affinity for type_def Nikita Pettik 2018-12-29 17:42 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-16 14:26 ` n.pettik 2018-12-28 9:34 ` [tarantool-patches] [PATCH 3/8] sql: remove numeric affinity Nikita Pettik 2018-12-29 9:01 ` [tarantool-patches] " Konstantin Osipov 2018-12-29 17:42 ` Vladislav Shpilevoy 2019-01-09 8:26 ` Konstantin Osipov 2019-01-16 14:26 ` n.pettik 2019-01-22 15:41 ` Vladislav Shpilevoy 2019-01-28 16:39 ` n.pettik 2019-01-30 13:04 ` Vladislav Shpilevoy 2019-02-01 16:39 ` n.pettik 2019-01-09 8:20 ` Konstantin Osipov 2018-12-28 9:34 ` [tarantool-patches] [PATCH 4/8] sql: replace affinity with field type for func Nikita Pettik 2018-12-28 9:34 ` [tarantool-patches] [PATCH 5/8] sql: replace field type with affinity for VDBE runtime Nikita Pettik 2018-12-29 17:42 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-16 14:26 ` n.pettik 2019-01-22 15:41 ` Vladislav Shpilevoy 2019-01-28 16:39 ` n.pettik 2019-01-30 13:04 ` Vladislav Shpilevoy 2019-02-01 16:39 ` n.pettik 2019-02-05 15:08 ` Vladislav Shpilevoy 2019-02-05 17:46 ` n.pettik 2018-12-28 9:34 ` [tarantool-patches] [PATCH 6/8] sql: replace affinity with field type in struct Expr Nikita Pettik 2018-12-29 17:42 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-16 14:26 ` n.pettik 2019-01-22 15:41 ` Vladislav Shpilevoy 2019-01-28 16:39 ` n.pettik 2019-01-30 13:04 ` Vladislav Shpilevoy [this message] 2019-02-01 16:39 ` n.pettik 2019-02-05 15:08 ` Vladislav Shpilevoy 2019-02-05 17:46 ` n.pettik 2018-12-28 9:34 ` [tarantool-patches] [PATCH 7/8] sql: clean-up affinity from SQL source code Nikita Pettik 2018-12-29 17:42 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-16 14:26 ` n.pettik 2019-01-22 15:41 ` Vladislav Shpilevoy 2019-01-28 16:40 ` n.pettik 2019-01-30 13:04 ` Vladislav Shpilevoy 2019-02-01 16:39 ` n.pettik 2019-02-05 15:08 ` Vladislav Shpilevoy 2019-02-05 17:46 ` n.pettik 2018-12-28 9:34 ` [tarantool-patches] [PATCH 8/8] Remove affinity from field definition Nikita Pettik 2019-02-05 19:41 ` [tarantool-patches] Re: [PATCH 0/8] Eliminate affinity from source code Vladislav Shpilevoy 2019-02-08 13:37 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=aa4d3548-87bc-d738-8c65-84a51cacf6e7@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH 6/8] sql: replace affinity with field type in struct Expr' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox