[tarantool-patches] Re: [PATCH 6/8] sql: replace affinity with field type in struct Expr
n.pettik
korablev at tarantool.org
Tue Feb 5 20:46:21 MSK 2019
>> 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.
Well, that is actually true. See below, I removed this function at all.
>
>> 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?
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 …_type2(field_type, expr_cmp_mutual_type(expr));
I’ve made comparative table and …_type2(expr_cmp_mutal_type, field_type)
gives us the same results as sql_expr_type_is_compatible.
Hence, I will remove this function and use …_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;
}
-/**
- * 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 == TK_EQ || pExpr->op == TK_IN || pExpr->op == TK_LT ||
@@ -278,25 +273,6 @@ expr_cmp_mutual_type(struct Expr *pExpr)
return type;
}
-/**
- * @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.
- */
-bool
-sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type field_type)
-{
- 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;
- 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);
-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) == 0) {
pX = pTerm->pExpr;
- if (!sql_expr_cmp_type_is_compatible(pX, pScan->idx_type))
+ enum field_type expr_type =
+ expr_cmp_mutual_type(pX);
+ if (!field_type1_contains_type2(expr_type, pScan->idx_type))
continue;
if (pScan->is_column_seen) {
Parse *pParse =
@@ -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 != pSrc->iCursor)
return 0;
if ((pTerm->eOperator & WO_EQ) == 0)
@@ -699,8 +700,9 @@ termCanDriveIndex(WhereTerm * pTerm, /* WHERE clause term to check */
return 0;
if (pTerm->u.leftColumn < 0)
return 0;
- aff = pSrc->pTab->aCol[pTerm->u.leftColumn].affinity;
- if (!sql_expr_cmp_type_is_compatible(pTerm->pExpr, aff))
+ enum field_type type = pSrc->pTab->def->fields[pTerm->u.leftColumn].type;
+ enum field_type expr_type = 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 = 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:
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 = 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;
>> 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’.
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);
>
> 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);
More information about the Tarantool-patches
mailing list