[tarantool-patches] Re: [PATCH 6/8] sql: replace affinity with field type in struct Expr
n.pettik
korablev at tarantool.org
Fri Feb 1 19:39:04 MSK 2019
>>>>> 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
>>>
>>> I've already acked the patch. Please, hurry up Kirill Y. to push it,
>>> rebase this branch, and replace ANY with SCALAR.
>> I had to add some other fixes to make this happen:
>> Changes to sql: replace affinity with field type for func:
>> diff --git a/test-run b/test-run
>> index 02207efd2..4c3f11812 160000
>> --- a/test-run
>> +++ b/test-run
>> @@ -1 +1 @@
>> -Subproject commit 02207efd2ca44067b76c79bf012f142016d929ae
>> +Subproject commit 4c3f11812c0c17f08780f31b6a748eef1126b2e2
>
> 1. ???
Accidentally trapped to diff.
>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index fdf00273a..e1a0dfa73 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -92,16 +111,14 @@ sql_expr_type(struct Expr *pExpr)
>> * return INTEGER.
>> */
>> return FIELD_TYPE_INTEGER;
>> - }
>> - /*
>> - * In case of unary plus we shouldn't discard
>> - * type of operand (since plus always features
>> - * NUMERIC type).
>> - */
>> - if (op == TK_UPLUS) {
>> + case TK_UMINUS:
>> + case TK_UPLUS:
>> + case TK_NO:
>> + case TK_BITNOT:
>> assert(pExpr->pRight == NULL);
>> - return pExpr->pLeft->type;
>> + return sql_expr_type(pExpr->pLeft);
>> }
>> +
>
> 2. Stray empty line.
Removed:
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index e1a0dfa73..4ff7169ab 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -118,7 +118,6 @@ sql_expr_type(struct Expr *pExpr)
assert(pExpr->pRight == NULL);
return sql_expr_type(pExpr->pLeft);
}
-
return pExpr->type;
}
>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index d263c1bb5..e1a0dfa73 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -81,27 +57,46 @@ sqlite3ExprAffinity(Expr * pExpr)
>> case TK_SELECT:
>> assert(pExpr->flags & EP_xIsSelect);
>> el = pExpr->x.pSelect->pEList;
>> - return sqlite3ExprAffinity(el->a[0].pExpr);
>> + return sql_expr_type(el->a[0].pExpr);
>> case TK_CAST:
>> assert(!ExprHasProperty(pExpr, EP_IntValue));
>> - return pExpr->affinity;
>> + return pExpr->type;
>> case TK_AGG_COLUMN:
>> case TK_COLUMN:
>> + case TK_TRIGGER:
>> assert(pExpr->iColumn >= 0);
>> - return sqlite3TableColumnAffinity(pExpr->space_def,
>> - pExpr->iColumn);
>> + return pExpr->space_def->fields[pExpr->iColumn].type;
>> case TK_SELECT_COLUMN:
>> assert(pExpr->pLeft->flags & EP_xIsSelect);
>> el = pExpr->pLeft->x.pSelect->pEList;
>> - return sqlite3ExprAffinity(el->a[pExpr->iColumn].pExpr);
>> + return sql_expr_type(el->a[pExpr->iColumn].pExpr);
>> case TK_PLUS:
>> case TK_MINUS:
>> case TK_STAR:
>> case TK_SLASH:
>> + case TK_REM:
>> + case TK_BITAND:
>> + case TK_BITOR:
>> + case TK_LSHIFT:
>> + case TK_RSHIFT:
>> assert(pExpr->pRight != NULL && pExpr->pLeft != NULL);
>> - enum affinity_type lhs_aff = sqlite3ExprAffinity(pExpr->pLeft);
>> - enum affinity_type rhs_aff = sqlite3ExprAffinity(pExpr->pRight);
>> - return sql_affinity_result(rhs_aff, lhs_aff);
>> + enum field_type lhs_type = sql_expr_type(pExpr->pLeft);
>> + enum field_type rhs_type = sql_expr_type(pExpr->pRight);
>> + return sql_type_result(rhs_type, lhs_type);
>> + case TK_CONCAT:
>> + return FIELD_TYPE_STRING;
>
> 1. Indentation.
Fixed:
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index e1a0dfa73..91d61b2d9 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -84,7 +84,7 @@ sql_expr_type(struct Expr *pExpr)
enum field_type rhs_type = sql_expr_type(pExpr->pRight);
return sql_type_result(rhs_type, lhs_type);
case TK_CONCAT:
- return FIELD_TYPE_STRING;
+ return FIELD_TYPE_STRING;
case TK_CASE:
>> @@ -252,74 +245,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)
>> +enum field_type
>> +sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type field_type)
>
> 2. Returns 'bool true', but the return value type is enum field_type?
> Please, change to bool.
Fixed:
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index e1a0dfa73..2bc4f00cf 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -285,7 +284,7 @@ expr_cmp_mutual_type(struct Expr *pExpr)
* @retval Return true if the index with @field_type may be used to
* implement the comparison in expr.
*/
-enum field_type
+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 1;
+ return true;
case FIELD_TYPE_STRING:
return field_type == FIELD_TYPE_STRING;
default:
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index d892631d8..8f75546de 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4284,7 +4284,7 @@ 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);
-enum field_type
+bool
sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type idx_type);
>> {
>> - char aff = comparisonAffinity(pExpr);
>> - switch (aff) {
>> - case AFFINITY_BLOB:
>> + enum field_type type = expr_cmp_mutual_type(expr);
>> + switch (type) {
>> + case FIELD_TYPE_SCALAR:
>> return 1;
>
> 3. 1 -> true
Fixed, see above.
>> @@ -2140,21 +2116,11 @@ sqlite3ExprCanBeNull(const Expr * p)
>> }
>> }
>> -/*
>> - * 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
>> -sqlite3ExprNeedsNoAffinityChange(const Expr * p, char aff)
>> +sql_expr_needs_no_type_change(const struct Expr *p, enum field_type type)
>
> 4. The same. Anyway you changed the function name, so without
> increasing the diff you can change its return value to bool,
> and the values below 0 -> false, 1 -> true.
That’s true. Fixed:
@@ -2116,12 +2115,12 @@ sqlite3ExprCanBeNull(const Expr * p)
}
}
-int
+bool
sql_expr_needs_no_type_change(const struct Expr *p, enum field_type type)
{
u8 op;
if (type == FIELD_TYPE_SCALAR)
- return 1;
+ return true;
while (p->op == TK_UPLUS || p->op == TK_UMINUS) {
p = p->pLeft;
}
@@ -2136,13 +2135,13 @@ sql_expr_needs_no_type_change(const struct Expr *p, enum field_type type)
case TK_STRING:
return type == FIELD_TYPE_STRING;
case TK_BLOB:
- return 1;
+ return true;
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:
- return 0;
+ return false;
}
}
index d892631d8..f200752ce 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3734,7 +3734,7 @@ int sqlite3ExprCanBeNull(const Expr *);
* is harmless. A false positive, however, can result in the wrong
* answer.
*/
-int
+bool
sql_expr_needs_no_type_change(const struct Expr *expr, enum field_type type);
More information about the Tarantool-patches
mailing list