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 98B8E250CE for ; Tue, 5 Feb 2019 10:08: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 Wen4yQWDQil3 for ; Tue, 5 Feb 2019 10:08:24 -0500 (EST) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 53E0A20C25 for ; Tue, 5 Feb 2019 10:08:24 -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> <3256389D-8BEA-4FB2-B214-D4F66067F333@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Tue, 5 Feb 2019 18:08:20 +0300 MIME-Version: 1.0 In-Reply-To: <3256389D-8BEA-4FB2-B214-D4F66067F333@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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, "n.pettik" Thanks for the fixes! See 5 comments below. > 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. '=', '<', 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) > +bool > +sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type field_type) > { > - char aff = comparisonAffinity(pExpr); > - switch (aff) { > - case AFFINITY_BLOB: > - return 1; > - case AFFINITY_TEXT: > - return idx_affinity == AFFINITY_TEXT; > + enum field_type type = expr_cmp_mutual_type(expr); > + switch (type) { > + case FIELD_TYPE_SCALAR: > + return true; > + case FIELD_TYPE_STRING: > + return field_type == FIELD_TYPE_STRING; 1. But type is never == FIELD_TYPE_STRING here. Because expr_cmp_mutual_type() returns either SCALAR or NUMBER. I added an assertion type != FIELD_TYPE_STRING and it did not fail in tests. > default: > - return sqlite3IsNumericAffinity(idx_affinity); > + return sql_type_is_numeric(field_type); 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 field_type1_contains_type2(field_type, expr_cmp_mutual_type(expr)); 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: field_type1_contains_type2(expr_cmp_mutual_type(expr), field_type); And the tests passed. Why? > } > } > > 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 = pEList->a[i].pExpr; > if (NEVER(p == 0)) > continue; > - switch (p->affinity) { > - case AFFINITY_INTEGER: > + if (p->op == TK_VARIABLE) > + var_pos[var_count++] = i; 3. Why did you move these two lines out of 'switch:' clause? As I understand, for field types INTEGER, NUMBER, STRING, SCALAR op never == 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: 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 = pEList->a[i].pExpr; if (NEVER(p == 0)) continue; - if (p->op == TK_VARIABLE) - var_pos[var_count++] = 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 == TK_VARIABLE) + var_pos[var_count++] = i; sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "BOOLEAN", SQLITE_TRANSIENT); break; > + switch (p->type) { > + case FIELD_TYPE_INTEGER: > sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "INTEGER", > SQLITE_TRANSIENT); > break; > - case AFFINITY_REAL: > + case FIELD_TYPE_NUMBER: > sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "NUMERIC", > SQLITE_TRANSIENT); > break; > - case AFFINITY_TEXT: > + case FIELD_TYPE_STRING: > sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "TEXT", > SQLITE_TRANSIENT); > break; > - case AFFINITY_BLOB: > + case FIELD_TYPE_SCALAR: > sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "BLOB", > SQLITE_TRANSIENT); > break; > - default: ; > - char *type; > - /* > - * For variables we set BOOLEAN type since > - * unassigned bindings will be replaced > - * with NULL automatically, i.e. without > - * explicit call of sql_bind_value(). > - */ > - if (p->op == TK_VARIABLE) { > - var_pos[var_count++] = i; > - type = "BOOLEAN"; > - } else { > - type = "UNKNOWN"; > - } > - sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, type, > + case FIELD_TYPE_BOOLEAN: > + sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "BOOLEAN", > + SQLITE_TRANSIENT); > + break; > + default: > + sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "UNKNOWN", > SQLITE_TRANSIENT); > } > if (pEList->a[i].zName) { > 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. */ 4. 'Type' -> 'type'. > + enum field_type type; > /** Conflict action for RAISE() function. */ > enum on_conflict_action on_conflict_action; > }; > @@ -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); 5. Expr -> struct Expr.