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 22A782620B for ; Tue, 5 Feb 2019 12:46:24 -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 95LzAWewxX9W for ; Tue, 5 Feb 2019 12:46:24 -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 A19442619D for ; Tue, 5 Feb 2019 12:46:23 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH 6/8] sql: replace affinity with field type in struct Expr From: "n.pettik" In-Reply-To: Date: Tue, 5 Feb 2019 20:46:21 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <1F427E20-6022-4D86-8CE0-15BC01939CBF@tarantool.org> 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> <3256389D-8BEA-4FB2-B214-D4F66067F333@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 >> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c >> index 5676d0720..db8577d1c 100644 >> --- a/src/box/sql/expr.c >> +++ b/src/box/sql/expr.c >> @@ -252,74 +244,57 @@ sql_expr_coll(Parse *parse, Expr *p, bool = *is_explicit_coll, uint32_t *coll_id) >> -/* >> - * 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 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) >> +bool >> +sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type = field_type) >> { >> - char aff =3D comparisonAffinity(pExpr); >> - switch (aff) { >> - case AFFINITY_BLOB: >> - return 1; >> - case AFFINITY_TEXT: >> - return idx_affinity =3D=3D AFFINITY_TEXT; >> + enum field_type type =3D expr_cmp_mutual_type(expr); >> + switch (type) { >> + case FIELD_TYPE_SCALAR: >> + return true; >> + case FIELD_TYPE_STRING: >> + return field_type =3D=3D FIELD_TYPE_STRING; >=20 > 1. But type is never =3D=3D FIELD_TYPE_STRING here. Because > expr_cmp_mutual_type() returns either SCALAR or NUMBER. > I added an assertion type !=3D FIELD_TYPE_STRING and it did > not fail in tests. Well, that is actually true. See below, I removed this function at all. >=20 >> default: >> - return sqlite3IsNumericAffinity(idx_affinity); >> + return sql_type_is_numeric(field_type); >=20 > 2. Also, I do not understand, why not to just use > field_type1_contains_type2(). sql_expr_cmp_type_is_compatible() > says, that it returns true, if a column of type field_type can > be used to "implement comparison" in an expression. So > this is the same as >=20 > field_type1_contains_type2(field_type, expr_cmp_mutual_type(expr)); >=20 > If I correctly understood what does it mean - "implement comparison". > Then I tried to replace sql_expr_cmp_type_is_compatible() with the > call above and the tests failed. Then I swapped the arguments: >=20 > field_type1_contains_type2(expr_cmp_mutual_type(expr), field_type); >=20 > And the tests passed. Why? I guess because expr_cmp_mutal_type returns SCALAR or NUMBER as you noticed. The first one is compatible with all scalar types and = not vice versa. So ..._type2(expr_cmp_mutual_type(expr), field_type) is not the same as =E2=80=A6_type2(field_type, expr_cmp_mutual_type(expr)); I=E2=80=99ve made comparative table and =E2=80=A6_type2(expr_cmp_mutal_typ= e, field_type) gives us the same results as sql_expr_type_is_compatible. Hence, I will remove this function and use =E2=80=A6_type2() instead. Diff: diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index db8577d1c..36da1ecdb 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -252,12 +252,7 @@ sql_type_result(enum field_type lhs, enum = field_type rhs) return FIELD_TYPE_SCALAR; } =20 -/** - * 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 +enum field_type 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 || @@ -278,25 +273,6 @@ expr_cmp_mutual_type(struct Expr *pExpr) return type; } =20 -/** - * @param expr is a comparison expression, eg. '=3D', '<', 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. - */ -bool -sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type = field_type) -{ - enum field_type type =3D expr_cmp_mutual_type(expr); - switch (type) { - case FIELD_TYPE_SCALAR: - return true; - case FIELD_TYPE_STRING: - return field_type =3D=3D FIELD_TYPE_STRING; - default: - return sql_type_is_numeric(field_type); - } -} diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 047a77b68..61f316550 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -4284,8 +4284,13 @@ sql_emit_table_types(struct Vdbe *v, struct = space_def *def, int reg); enum field_type sql_type_result(enum field_type lhs, enum field_type rhs); =20 -bool -sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type = idx_type); +/** + * pExpr is a comparison operator. Return the type affinity + * that should be applied to both operands prior to doing + * the comparison. + */ +enum field_type +expr_cmp_mutual_type(struct Expr *pExpr); diff --git a/src/box/sql/where.c b/src/box/sql/where.c index 219009ef8..6a47f8c99 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -298,7 +298,9 @@ whereScanNext(WhereScan * pScan) /* Verify the affinity = and collating sequence match */ if ((pTerm->eOperator & = WO_ISNULL) =3D=3D 0) { pX =3D = pTerm->pExpr; - if = (!sql_expr_cmp_type_is_compatible(pX, pScan->idx_type)) + enum field_type = expr_type =3D + = expr_cmp_mutual_type(pX); + if = (!field_type1_contains_type2(expr_type, pScan->idx_type)) = continue; if = (pScan->is_column_seen) { Parse = *pParse =3D @@ -690,7 +692,6 @@ termCanDriveIndex(WhereTerm * pTerm, /* WHERE = clause term to check */ Bitmask notReady /* Tables in outer loops of the = join */ ) { - char aff; if (pTerm->leftCursor !=3D pSrc->iCursor) return 0; if ((pTerm->eOperator & WO_EQ) =3D=3D 0) @@ -699,8 +700,9 @@ termCanDriveIndex(WhereTerm * pTerm, /* WHERE = clause term to check */ return 0; if (pTerm->u.leftColumn < 0) return 0; - aff =3D pSrc->pTab->aCol[pTerm->u.leftColumn].affinity; - if (!sql_expr_cmp_type_is_compatible(pTerm->pExpr, aff)) + enum field_type type =3D = pSrc->pTab->def->fields[pTerm->u.leftColumn].type; + enum field_type expr_type =3D = expr_cmp_mutual_type(pTerm->pExpr); + if (!field_type1_contains_type2(expr_type, type)) return 0; return 1; } (The last change is related to dead code). >> diff --git a/src/box/sql/select.c b/src/box/sql/select.c >> index ef4514374..9843eb4a5 100644 >> --- a/src/box/sql/select.c >> +++ b/src/box/sql/select.c >> @@ -1728,38 +1727,31 @@ generateColumnNames(Parse * pParse, /* = Parser context */ >> p =3D pEList->a[i].pExpr; >> if (NEVER(p =3D=3D 0)) >> continue; >> - switch (p->affinity) { >> - case AFFINITY_INTEGER: >> + if (p->op =3D=3D TK_VARIABLE) >> + var_pos[var_count++] =3D i; >=20 > 3. Why did you move these two lines out of 'switch:' clause? As I > understand, for field types INTEGER, NUMBER, STRING, SCALAR op > never =3D=3D TK_VARIABLE. So this check now executes mostly without > necessity, and should be done only for FIELD_TYPE_BOOLEAN. I > did this and the tests passed: You are absolutely right, just wanted to make it look better (indeed, not the best try). Returned back: diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 9843eb4a5..d8ab9a40d 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -1727,8 +1727,6 @@ generateColumnNames(Parse * pParse, /* = Parser context */ p =3D pEList->a[i].pExpr; if (NEVER(p =3D=3D 0)) continue; - if (p->op =3D=3D TK_VARIABLE) - var_pos[var_count++] =3D i; switch (p->type) { case FIELD_TYPE_INTEGER: sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, = "INTEGER", @@ -1747,6 +1745,8 @@ generateColumnNames(Parse * pParse, /* = Parser context */ SQLITE_TRANSIENT); break; case FIELD_TYPE_BOOLEAN: + if (p->op =3D=3D TK_VARIABLE) + var_pos[var_count++] =3D i; sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, = "BOOLEAN", SQLITE_TRANSIENT); break; >> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h >> index 79999509c..047a77b68 100644 >> --- a/src/box/sql/sqliteInt.h >> +++ b/src/box/sql/sqliteInt.h >> @@ -2091,8 +2093,8 @@ typedef int ynVar; >> struct Expr { >> u8 op; /* Operation performed by this node */ >> union { >> - /** The affinity of the column. */ >> - enum affinity_type affinity; >> + /** The Type of the column. */ >=20 > 4. 'Type' -> 'type=E2=80=99. diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 047a77b68..6e4ead573 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 Type of the column. */ + /** The type of the column. */ >> @@ -4267,26 +4281,30 @@ sql_index_type_str(struct sqlite3 *db, const = struct index_def *idx_def); >> /** >> - * 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. >> + * >> + * 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); >=20 > 5. Expr -> struct Expr. @@ -4304,7 +4304,7 @@ sql_expr_cmp_type_is_compatible(struct Expr *expr, = enum field_type idx_type); * SELECT * FROM t1 WHERE (select a from t1); */ enum field_type -sql_expr_type(Expr *pExpr); +sql_expr_type(struct Expr *pExpr);