[tarantool-patches] Re: [PATCH 4/9] sql: introduce type boolean

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes!

>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>> index 6b38e8e66..d70b64c45 100644
>>> --- a/src/box/sql/expr.c
>>> +++ b/src/box/sql/expr.c
>>> @@ -3775,7 +3766,7 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>>>                }
>>>        case TK_TRUE:
>>>        case TK_FALSE: {
>>> -                       vdbe_emit_bool(pParse->pVdbe, pExpr, target);
>>> +                       sqlVdbeAddOp2(v, OP_Bool, op == TK_TRUE, target);
>>>                        return target;
>>>                }
>>>
>>>> 6. I see, that all other 'case's set pExpr->type. Why don't you
>>>> do it here?
>>>
>>> It is required only for non-constant values. For literals it is assigned
>>> in parse.y spanExpr().
>>
>> 1. Then why case TK_INTEGER sets 'pExpr->type = FIELD_TYPE_INTEGER',
>> TK_STRING sets 'pExpr->type = FIELD_TYPE_STRING;', TK_BLOB sets
>> 'pExpr->type = FIELD_TYPE_SCALAR;' - I see all of them being set in
>> parse.y.
> 
> Those are literals, so as I said above, their types are assigned
> right during parsing. If you are asking “why do we need to do so?”,
> then answer is “they are leaf nodes and it is most appropriate
> moment to do so”.

No, my question was not about why they are set in parse.y, but why
are they set in sqlExprCodeTarget() again, after parse.y. But I've just
noticed, that the next patch "sql: improve type determination for column meta"
fixes that.

>>>> 8. Looking at all these type comparison prohibitions I am getting
>>>> afraid of how will we select from SCALAR columns?
>>>>
>>>> A schema:
>>>>
>>>>   box.execute('CREATE TABLE t (id INTEGER PRIMARY KEY, a SCALAR UNIQUE);')
>>>>   box.execute("INSERT INTO t VALUES (1, 1), (2, true), (3, false), (4, 'str')")
>>>>
>>>> SQL select:
>>>>
>>>>> box.execute('SELECT * FROM t WHERE a > 1')
>>>>   ---
>>>>   - error: 'Type mismatch: can not convert INTEGER to boolean'
>>>>   ...
>>>>
>>>> The same Lua select:
>>>>
>>>>> box.space.T.index[1]:select({1}, {iterator = 'GT'})
>>>>   ---
>>>>   - - [4, 'str']
>>>>   ...
>>>>
>>>> In fact, now we can not use SCALAR in SQL for any comparisons
>>>> because it will raise type mismatch on literally everything.
>>>>
>>>> What are we going to do with it? IMO, we could add a flag like
>>>> 'is_scalar' to struct Mem in its flags section, which would allow
>>>> to compare this value with anything.
>>>
>>> Konstantin suggested to extend struct Mem with field_type,
>>> which is going to substitute this flag.
>>>
>>>> Looks crutchy though. And is
>>>> not a part of this issue of course, but should be filed into the
>>>> issue list.
>>>>
>>>> I see a similar issue https://github.com/tarantool/tarantool/issues/4124.
>>>> Probably, it is worth extending it instead of opening new.
>>>
>>> I consider this as a trade-off of using scalar: users shouldn’t use this
>>> type until they are really have to. Nevertheless, they still can use CAST
>>> operator and switch case, or write their own lua-function converting values
>>> to required type.
>>
>> 2. Users may have good designed schema and work in each request with only
>> one type in such a column, but still they will get errors just because
>> the iterator can not walk to the needed tree node. Lets modify my example:
>>
>>    SELECT * FROM t WHERE a = 1;
>>
>> This request implicitly says that it will work with numbers only, but
>> somehow its success depends on the content of the space, not even
>> related to '1' value. In a nutshell it means that insertion of something
>> into a space can break existing requests not related to the inserted
>> data. It is weird. Such scalar type would be useless.
> 
> Yep, we can observe the same behaviour in DB2. For instance:

DB2 is a perfect example when we deal with ANSI, but SCALAR is not ANSI
and is not the same as VARCHAR. Its single and main purpose to be
comparable with anything. If it can't be compared with anything, then it
is not SCALAR.

BTW, this VARCHAR case is also weird, even being the standard. What if
I used "1" instead of simple 1?


Please, consider these additions, on the branch:

======================================================================
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index ef1a6a6bb..e0930f05d 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2568,7 +2568,7 @@ case OP_IfNot: {            /* jump, in1 */
 	if (pIn1->flags & MEM_Null) {
 		c = pOp->p3;
 	} else if ((pIn1->flags & MEM_Bool) != 0) {
-		c = pOp->opcode==OP_IfNot ? ! pIn1->u.b : pIn1->u.b;
+		c = pOp->opcode == OP_IfNot ? ! pIn1->u.b : pIn1->u.b;
 	} else {
 		double v;
 		if (sqlVdbeRealValue(pIn1, &v) != 0) {
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 713a5c686..8c1aa4a8a 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -667,16 +667,14 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
 		return 0;
 	case FIELD_TYPE_BOOLEAN:
 		if ((pMem->flags & MEM_Int) != 0) {
-			pMem->u.b = pMem->u.i;
-			MemSetTypeFlag(pMem, MEM_Bool);
+			mem_set_bool(pMem, pMem->u.i);
 			return 0;
 		}
 		if ((pMem->flags & MEM_Str) != 0) {
 			bool value;
 			if (str_cast_to_boolean(pMem->z, &value) != 0)
 				return -1;
-			MemSetTypeFlag(pMem, MEM_Bool);
-			pMem->u.b = value;
+			mem_set_bool(pMem, value);
 			return 0;
 		}
 		if ((pMem->flags & MEM_Bool) != 0)