[tarantool-patches] Re: [PATCH 6/8] sql: replace affinity with field type in struct Expr
n.pettik
korablev at tarantool.org
Wed Jan 16 17:26:24 MSK 2019
>> -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 != FIELD_TYPE_ANY && rhs != FIELD_TYPE_ANY) {
>> + /*
>> + * Both sides of the comparison are columns.
>> + * If one has numeric type, use that.
>> */
>
> 1. The comment is useless IMO. It just repeats exactly the
> same obvious thing what is written in the next line.
Ok, I’d 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 != FIELD_TYPE_ANY && rhs != FIELD_TYPE_ANY) {
- /*
- * Both sides of the comparison are columns.
- * If one has numeric type, use that.
- */
>
>> - 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;
>> +
>
> 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 == FIELD_TYPE_ANY && rhs == FIELD_TYPE_ANY) {
/*
* Neither side of the comparison is a column.
>
>> + } else if (lhs == FIELD_TYPE_ANY && rhs == 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 == 0 || aff2 == 0);
>> - return (aff1 + aff2);
>> + if (lhs == 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)
>
> 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 == TK_EQ || pExpr->op == TK_IN || pExpr->op == TK_LT ||
pExpr->op == TK_GT || pExpr->op == TK_GE || pExpr->op == TK_LE ||
>> -/*
>> - * 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 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)
>
> 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’s 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 == TK_EQ || pExpr->op == TK_IN || pExpr->op == TK_LT ||
pExpr->op == TK_GT || pExpr->op == TK_GE || pExpr->op == 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.
>
> Maybe rename to something like 'sql_expr_cmp_type_is_possible’ ?
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 = comparisonAffinity(expr);
+ enum field_type type = 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);
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);
/**
* 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) == 0) {
pX = 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 =
>
>> {
>> - char aff = comparisonAffinity(pExpr);
>> - switch (aff) {
>> - case AFFINITY_BLOB:
>> + enum field_type type = comparisonAffinity(expr);
>> + switch (type) {
>> + case FIELD_TYPE_SCALAR:
>> return 1;
>> - case AFFINITY_TEXT:
>> - return idx_affinity == AFFINITY_TEXT;
>> + case FIELD_TYPE_STRING:
>> + return idx_type == 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)
>
> 5. Name is changed from negative to positive, but the logic is
> the same, why? Original was 'NoChange', now it is 'needs_change’.
My fault, missed ’no’ 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 *);
+/**
+ * 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 = sqlite3VectorFieldSubexpr(pRight, i);
enum field_type expr_type = sql_expr_type(p);
if (sql_type_result(expr_type, type) == FIELD_TYPE_SCALAR ||
- sql_expr_needs_type_change(p, type)) {
+ sql_expr_needs_no_type_change(p, type)) {
zAff[i] = AFFINITY_BLOB;
}
}
@@ -788,7 +788,7 @@ codeAllEqualityTerms(Parse * pParse, /* Parsing context */
FIELD_TYPE_SCALAR) {
zAff[j] = AFFINITY_BLOB;
}
- if (sql_expr_needs_type_change(pRight, idx_type))
+ if (sql_expr_needs_no_type_change(pRight, idx_type))
zAff[j] = AFFINITY_BLOB;
}
> Also, please, move the function comment to the declaration in sqliteInt.h.
Done, see above.
>
>> {
>> 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;
>> @@ -2167,27 +2142,21 @@ sqlite3ExprNeedsNoAffinityChange(const Expr * p, char aff)
>> if (op == TK_REGISTER)
>> op = p->op2;
>> switch (op) {
>> - case TK_INTEGER:{
>> - return aff == AFFINITY_INTEGER;
>> - }
>> - case TK_FLOAT:{
>> - return aff == AFFINITY_REAL;
>> - }
>> - case TK_STRING:{
>> - return aff == AFFINITY_TEXT;
>> - }
>> - case TK_BLOB:{
>> - return 1;
>> - }
>> + case TK_INTEGER:
>> + return type == FIELD_TYPE_INTEGER;
>> + case TK_FLOAT:
>> + return type == FIELD_TYPE_NUMBER;
>> + case TK_STRING:
>> + return type == FIELD_TYPE_STRING;
>> + case TK_BLOB:
>> + return 1;
>> case TK_COLUMN:{
>> - assert(p->iTable >= 0); /* p cannot be part of a CHECK constraint */
>> - return p->iColumn < 0
>> - && (aff == AFFINITY_INTEGER
>> - || aff == AFFINITY_REAL);
>> - }
>> - default:{
>> - return 0;
>> + /* p cannot be part of a CHECK constraint. */
>> + assert(p->iTable >= 0);
>> + return p->iColumn < 0 && sql_type_is_numeric(type);
>> }
>
> 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 == FIELD_TYPE_STRING;
case TK_BLOB:
return 1;
- case TK_COLUMN:{
- /* p cannot be part of a CHECK constraint. */
- assert(p->iTable >= 0);
- return p->iColumn < 0 && sql_type_is_numeric(type);
- }
+ case TK_COLUMN:
+ /* p cannot be part of a CHECK constraint. */
+ assert(p->iTable >= 0);
+ return p->iColumn < 0 && sql_type_is_numeric(type);
default:
>
>> + default:
>> + return 0;
>> }
>> }
>> @@ -2826,16 +2795,15 @@ sqlite3CodeSubselect(Parse * pParse, /* Parsing context */
>> * that columns affinity when building index keys. If <expr> is not
>> * a column, use numeric affinity.
>> */
>> - char affinity; /* Affinity of the LHS of the IN */
>> int i;
>> ExprList *pList = pExpr->x.pList;
>> struct ExprList_item *pItem;
>> int r1, r2, r3;
>> - affinity = sqlite3ExprAffinity(pLeft);
>> - if (!affinity) {
>> - affinity = AFFINITY_BLOB;
>> - }
>> + enum field_type lhs_type =
>> + sql_expr_type(pLeft);
>> + if (lhs_type == FIELD_TYPE_ANY)
>> + lhs_type = FIELD_TYPE_SCALAR;
>
> 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
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’t ignore this type even now (IMHO).
>
>> 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. */
>
> 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. */
>
>> - 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.
>
> 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.
>
>> + *
>> + * 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] == AFFINITY_BLOB) {
>
> 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] == AFFINITY_BLOB) {
n--;
}
More information about the Tarantool-patches
mailing list