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 E95442576F for ; Wed, 16 Jan 2019 09:26:26 -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 ioCkKgYCXb0V for ; Wed, 16 Jan 2019 09:26:26 -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 7925A2569E for ; Wed, 16 Jan 2019 09:26:26 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH 6/8] sql: replace affinity with field type in struct Expr From: "n.pettik" In-Reply-To: <406a1cab-d3cd-437b-668e-141697fbd637@tarantool.org> Date: Wed, 16 Jan 2019 17:26:24 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <8D9E26B4-FC3D-4EBD-9941-5570A5ECFA17@tarantool.org> References: <406a1cab-d3cd-437b-668e-141697fbd637@tarantool.org> 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: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy >> -enum affinity_type >> -sql_affinity_result(enum affinity_type aff1, enum affinity_type = aff2) >> +enum field_type >> +sql_type_result(enum field_type lhs, enum field_type rhs) >> { >> - if (aff1 && aff2) { >> - /* Both sides of the comparison are columns. If one has = numeric >> - * affinity, use that. Otherwise use no affinity. >> + if (lhs !=3D FIELD_TYPE_ANY && rhs !=3D FIELD_TYPE_ANY) { >> + /* >> + * Both sides of the comparison are columns. >> + * If one has numeric type, use that. >> */ >=20 > 1. The comment is useless IMO. It just repeats exactly the > same obvious thing what is written in the next line. Ok, I=E2=80=99d rather agree. I simply copied that comment from original SQLite function. Now removed: diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 4816310c3..ba7269d08 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -235,15 +235,10 @@ enum field_type sql_type_result(enum field_type lhs, enum field_type rhs) { if (lhs !=3D FIELD_TYPE_ANY && rhs !=3D FIELD_TYPE_ANY) { - /* - * Both sides of the comparison are columns. - * If one has numeric type, use that. - */ >=20 >> - if (sqlite3IsNumericAffinity(aff1) >> - || sqlite3IsNumericAffinity(aff2)) { >> - return AFFINITY_REAL; >> - } else { >> - return AFFINITY_BLOB; >> - } >> - } else if (!aff1 && !aff2) { >> - /* Neither side of the comparison is a column. Compare = the >> - * results directly. >> + if (sql_type_is_numeric(lhs) || = sql_type_is_numeric(rhs)) >> + return FIELD_TYPE_NUMBER; >> + else >> + return FIELD_TYPE_SCALAR; >> + >=20 > 2. Unnecessary empty line. Removed: if (sql_type_is_numeric(lhs) || = sql_type_is_numeric(rhs)) return FIELD_TYPE_NUMBER; else return FIELD_TYPE_SCALAR; - } else if (lhs =3D=3D FIELD_TYPE_ANY && rhs =3D=3D = FIELD_TYPE_ANY) { /* * Neither side of the comparison is a column. >=20 >> + } else if (lhs =3D=3D FIELD_TYPE_ANY && rhs =3D=3D = FIELD_TYPE_ANY) { >> + /* >> + * Neither side of the comparison is a column. >> + * Compare the results directly. >> */ >> - return AFFINITY_BLOB; >> + return FIELD_TYPE_SCALAR; >> } else { >> - /* One side is a column, the other is not. Use the = columns affinity. */ >> - assert(aff1 =3D=3D 0 || aff2 =3D=3D 0); >> - return (aff1 + aff2); >> + if (lhs =3D=3D FIELD_TYPE_ANY) >> + return rhs; >> + return lhs; >> } >> } >> @@ -285,45 +261,44 @@ sql_affinity_result(enum affinity_type aff1, = enum affinity_type aff2) >> * pExpr is a comparison operator. Return the type affinity that = should >> * be applied to both operands prior to doing the comparison. >> */ >> -static char >> +static enum field_type >> comparisonAffinity(Expr * pExpr) >=20 > 3. But it is not affinity anymore. 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) +comparison_mutual_type(struct Expr *pExpr) { assert(pExpr->op =3D=3D TK_EQ || pExpr->op =3D=3D TK_IN || = pExpr->op =3D=3D TK_LT || pExpr->op =3D=3D TK_GT || pExpr->op =3D=3D TK_GE || = pExpr->op =3D=3D TK_LE || >> -/* >> - * pExpr is a comparison expression, eg. '=3D', '<', 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. '=3D', '<', IN(...) = etc. >> + * @param idx_affinity is the affinity of an indexed column. >> + * @retval Return true if the index with @idx_type may be used to >> + * implement the comparison in expr. >> */ >> -int >> -sqlite3IndexAffinityOk(Expr * pExpr, char idx_affinity) >> +enum field_type >> +sql_index_type_is_ok(struct Expr *expr, enum field_type idx_type) >=20 > 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=E2=80=99s 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 =3D=3D TK_EQ || pExpr->op =3D=3D TK_IN || = pExpr->op =3D=3D TK_LT || pExpr->op =3D=3D TK_GT || pExpr->op =3D=3D TK_GE || = pExpr->op =3D=3D TK_LE || > Also, sorry, but I hate this suffix '_ok' or 'Ok'. It just says > nothing about what this function does. As well as that it has > nothing to do with an index, but has 'index' in the name. >=20 > Maybe rename to something like 'sql_expr_cmp_type_is_possible=E2=80=99 = ? Done: @@ -289,9 +285,9 @@ comparisonAffinity(Expr * pExpr) * implement the comparison in expr. */ enum field_type -sql_index_type_is_ok(struct Expr *expr, enum field_type idx_type) +sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type = idx_type) { - enum field_type type =3D comparisonAffinity(expr); + enum field_type type =3D expr_cmp_mutual_type(expr); switch (type) { case FIELD_TYPE_SCALAR: return 1; diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 216e8d57f..7116f34c5 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -4261,7 +4261,7 @@ enum field_type sql_type_result(enum field_type lhs, enum field_type rhs); =20 enum field_type -sql_index_type_is_ok(struct Expr *expr, enum field_type idx_type); +sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type = idx_type); =20 /** * Return the type of the expression pExpr. diff --git a/src/box/sql/where.c b/src/box/sql/where.c index 54531a8a5..a2e55e8ac 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -298,7 +298,7 @@ whereScanNext(WhereScan * pScan) /* Verify the affinity = and collating sequence match */ if ((pTerm->eOperator & = WO_ISNULL) =3D=3D 0) { pX =3D = pTerm->pExpr; - if = (!sql_index_type_is_ok(pX, pScan->idx_type)) + if = (!sql_expr_cmp_type_is_compatible(pX, pScan->idx_type)) = continue; if = (pScan->is_column_seen) { Parse = *pParse =3D >=20 >> { >> - char aff =3D comparisonAffinity(pExpr); >> - switch (aff) { >> - case AFFINITY_BLOB: >> + enum field_type type =3D comparisonAffinity(expr); >> + switch (type) { >> + case FIELD_TYPE_SCALAR: >> return 1; >> - case AFFINITY_TEXT: >> - return idx_affinity =3D=3D AFFINITY_TEXT; >> + case FIELD_TYPE_STRING: >> + return idx_type =3D=3D FIELD_TYPE_STRING; >> default: >> - return sqlite3IsNumericAffinity(idx_affinity); >> + return sql_type_is_numeric(idx_type); >> } >> } >> @@ -2155,10 +2130,10 @@ sqlite3ExprCanBeNull(const Expr * p) >> * answer. >> */ >> int >> -sqlite3ExprNeedsNoAffinityChange(const Expr * p, char aff) >> +sql_expr_needs_type_change(const Expr *p, enum field_type type) >=20 > 5. Name is changed from negative to positive, but the logic is > the same, why? Original was 'NoChange', now it is 'needs_change=E2=80=99= . My fault, missed =E2=80=99no=E2=80=99 particle. Fixed: -/* - * 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_type_change(const Expr *p, enum field_type type) +sql_expr_needs_no_type_change(const Expr *p, enum field_type type) diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 216e8d57f..5541cb6b2 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3710,8 +3710,18 @@ int sqlite3ExprIsTableConstant(Expr *, int); int sqlite3ExprIsInteger(Expr *, int *); int sqlite3ExprCanBeNull(const Expr *); =20 +/** + * 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_type_change(const Expr *epr, enum field_type type); +sql_expr_needs_no_type_change(const Expr *epr, enum field_type type); diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c index fe5c56c07..98b55db7a 100644 --- a/src/box/sql/wherecode.c +++ b/src/box/sql/wherecode.c @@ -428,7 +428,7 @@ updateRangeAffinityStr(Expr * pRight, /* RHS = of comparison */ Expr *p =3D sqlite3VectorFieldSubexpr(pRight, i); enum field_type expr_type =3D sql_expr_type(p); if (sql_type_result(expr_type, type) =3D=3D = FIELD_TYPE_SCALAR || - sql_expr_needs_type_change(p, type)) { + sql_expr_needs_no_type_change(p, type)) { zAff[i] =3D AFFINITY_BLOB; } } @@ -788,7 +788,7 @@ codeAllEqualityTerms(Parse * pParse, /* = Parsing context */ FIELD_TYPE_SCALAR) { zAff[j] =3D AFFINITY_BLOB; } - if (sql_expr_needs_type_change(pRight, = idx_type)) + if = (sql_expr_needs_no_type_change(pRight, idx_type)) zAff[j] =3D AFFINITY_BLOB; } > Also, please, move the function comment to the declaration in = sqliteInt.h. Done, see above. >=20 >> { >> u8 op; >> - if (aff =3D=3D AFFINITY_BLOB) >> + if (type =3D=3D FIELD_TYPE_SCALAR) >> return 1; >> while (p->op =3D=3D TK_UPLUS || p->op =3D=3D TK_UMINUS) { >> p =3D p->pLeft; >> @@ -2167,27 +2142,21 @@ sqlite3ExprNeedsNoAffinityChange(const Expr * = p, char aff) >> if (op =3D=3D TK_REGISTER) >> op =3D p->op2; >> switch (op) { >> - case TK_INTEGER:{ >> - return aff =3D=3D AFFINITY_INTEGER; >> - } >> - case TK_FLOAT:{ >> - return aff =3D=3D AFFINITY_REAL; >> - } >> - case TK_STRING:{ >> - return aff =3D=3D AFFINITY_TEXT; >> - } >> - case TK_BLOB:{ >> - return 1; >> - } >> + case TK_INTEGER: >> + return type =3D=3D FIELD_TYPE_INTEGER; >> + case TK_FLOAT: >> + return type =3D=3D FIELD_TYPE_NUMBER; >> + case TK_STRING: >> + return type =3D=3D FIELD_TYPE_STRING; >> + case TK_BLOB: >> + return 1; >> case TK_COLUMN:{ >> - assert(p->iTable >=3D 0); /* p cannot be = part of a CHECK constraint */ >> - return p->iColumn < 0 >> - && (aff =3D=3D AFFINITY_INTEGER >> - || aff =3D=3D AFFINITY_REAL); >> - } >> - default:{ >> - return 0; >> + /* p cannot be part of a CHECK constraint. */ >> + assert(p->iTable >=3D 0); >> + return p->iColumn < 0 && = sql_type_is_numeric(type); >> } >=20 > 6. Please, remove these braces too, if you decided to refactor > sqlite3ExprNeedsNoAffinityChange. Ok: @@ -2150,11 +2136,10 @@ sql_expr_needs_type_change(const Expr *p, enum = field_type type) return type =3D=3D FIELD_TYPE_STRING; case TK_BLOB: return 1; - case TK_COLUMN:{ - /* p cannot be part of a CHECK constraint. */ - assert(p->iTable >=3D 0); - return p->iColumn < 0 && = sql_type_is_numeric(type); - } + case TK_COLUMN: + /* p cannot be part of a CHECK constraint. */ + assert(p->iTable >=3D 0); + return p->iColumn < 0 && sql_type_is_numeric(type); default: >=20 >> + 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 =3D pExpr->x.pList; >> struct ExprList_item *pItem; >> int r1, r2, r3; >> - affinity =3D sqlite3ExprAffinity(pLeft); >> - if (!affinity) { >> - affinity =3D AFFINITY_BLOB; >> - } >> + enum field_type lhs_type =3D >> + sql_expr_type(pLeft); >> + if (lhs_type =3D=3D FIELD_TYPE_ANY) >> + lhs_type =3D FIELD_TYPE_SCALAR; >=20 > 7. Why did not you move this conversion to sql_expr_type? I mean > ANY -> SCALAR. Unfortunately, right now I can=E2=80=99t 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 After it hits 2.1, I will replace SCALAR with ANY. > 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=E2=80=99t ignore this type even now (IMHO). >=20 >> bool unused; >> sql_expr_coll(pParse, pExpr->pLeft, >> &unused, = &key_info->parts[0].coll_id); >> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h >> index 690fa6431..7d1159345 100644 >> --- a/src/box/sql/sqliteInt.h >> +++ b/src/box/sql/sqliteInt.h >> @@ -2092,7 +2094,7 @@ struct Expr { >> u8 op; /* Operation performed by this node */ >> union { >> /** The affinity of the column. */ >=20 > 8. Not affinity anymore. Fixed: diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 216e8d57f..209e7bf0f 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -2093,7 +2093,7 @@ typedef int ynVar; struct Expr { u8 op; /* Operation performed by this node */ union { - /** The affinity of the column. */ + /** The Type of the column. */ >=20 >> - enum affinity_type affinity; >> + enum field_type type; >> /** Conflict action for RAISE() function. */ >> enum on_conflict_action on_conflict_action; >> }; >> @@ -4253,26 +4257,30 @@ sql_index_type_str(struct sqlite3 *db, const = struct index_def *idx_def); >> void >> sql_emit_table_types(struct Vdbe *v, struct space_def *def, int = reg); >> -/** >> - * Return superposition of two affinities. >> - * This may be required for determining resulting >> - * affinity of expressions like a + '2'. >> - */ >> -enum affinity_type >> -sql_affinity_result(enum affinity_type aff1, enum affinity_type = aff2); >> +enum field_type >> +sql_type_result(enum field_type lhs, enum field_type rhs); >> -int sqlite3IndexAffinityOk(Expr * pExpr, char idx_affinity); >> +enum field_type >> +sql_index_type_is_ok(struct Expr *expr, enum field_type idx_type); >> /** >> - * Return the affinity character for a single column of a table. >> - * @param def space definition. >> - * @param idx column index. >> - * @retval AFFINITY >> + * Return the type of the expression pExpr. >> + * >> + * If pExpr is a column, a reference to a column via an 'AS' alias, >> + * or a sub-select with a column as the return value, then the >> + * type of that column is returned. Otherwise, type ANY is returned, >> + * indicating that the expression can feature any type. >=20 > 9. Returning to point 7 - I do not think it should return ANY. Anyway > you convert it to SCALAR in all places. See explanation above. >=20 >> + * >> + * The WHERE clause expressions in the following statements all >> + * have an type: >> + * >> + * CREATE TABLE t1(a); >> + * SELECT * FROM t1 WHERE a; >> + * SELECT a AS b FROM t1 WHERE b; >> + * SELECT * FROM t1 WHERE (select a from t1); >> */ >> -char >> -sqlite3TableColumnAffinity(struct space_def *def, int idx); >> - >> -char sqlite3ExprAffinity(Expr * pExpr); >> +enum field_type >> +sql_expr_type(Expr *pExpr); >> /** >> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c >> index aae5d6617..06335bcf7 100644 >> --- a/src/box/sql/wherecode.c >> +++ b/src/box/sql/wherecode.c >> @@ -392,6 +392,7 @@ codeApplyAffinity(Parse * pParse, int base, int = n, char *zAff) >> base++; >> zAff++; >> } >> + >> while (n > 1 && zAff[n - 1] =3D=3D AFFINITY_BLOB) { >=20 > 10. Garbage diff. Fixed: diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c index fe5c56c07..33b860f36 100644 --- a/src/box/sql/wherecode.c +++ b/src/box/sql/wherecode.c @@ -392,7 +392,6 @@ codeApplyAffinity(Parse * pParse, int base, int n, = char *zAff) base++; zAff++; } - while (n > 1 && zAff[n - 1] =3D=3D AFFINITY_BLOB) { n--; }