[tarantool-patches] Re: [PATCH 9/9] sql: make <search condition> accept only boolean

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

Thanks for the fixes!

On 23/04/2019 22:59, n.pettik wrote:
> 
>> On 18/04/2019 20:55, n.pettik wrote:
>>>
>>>>> <search condition> is a predicate used as a part of WHERE and
>>>>> JOIN clauses. ANSI SQL states that <search condition> must
>>>>> accept only boolean arguments. In our SQL it is implemented as
>>>>> bytecode instruction OP_If which in turn carries out logic of
>>>>> conditional jump. Since it can be involved in executing other routines
>>>>> different from <search condition>, 
>>>>
>>>> 1. Which other routines? What is a valid case of OP_If with non-boolean
>>>> value in check?
>>>
>>> For instance, to verify that register containing LIMIT value is > 0.
>>
>> Yes, and this is almost the only case. What is more, it happens only once
>> per request, to check if LIMIT == 0 initially. Further it is decremented
>> and checked via OP_IfNotZero and OP_DecrJumpZero.
>>
>>> It is quite hard to track values which come to this opcode, so we
>>> can’t be sure that it always accepts booleans.
>>
>> It is hard, but without it
>>
>> 1) You can't be sure, that really all the search conditions
>>   are checked to be booleans;
>>
>> 2) It makes OP_If/IfNot slower, and they are called repeatedly in
>>   requests;
> 
> One branching worth nothing. Below you suggest to split opcode
> into two (fix me if I’m wrong), which in turn affects performance way much more.

The places which I proposed to split are called only once per request.
For example, OP_IfNot with iLimit was used for initial check that it is
not zero. All the next work with iLimit was being done via special
opcodes OP_DecrJumpZero and OP_IfNotZero.

On the other hand, if we branch inside OP_IfNot, we branch repeatedly,
in cycles, many times per request.

> 
>> 3) It adds one more flag SQL_BOOLREQ, which looks very crutchy.
> 
> IMHO it is matter of taste. Anyway, removed this flag.

You are right, it would have been, if OP_IfNot/OP_If had already
had some other flags. But you proposed to change the opcodes
dramatically, to add a new argument just for some minor cases.
And as a result it hid a bug with CASE-WHEN search condition.

>> It violates the standard. "Information technology —
>> Database languages — SQL — Part 2: Foundation (SQL/Foundation)",
>> 2011, page 230.
>>
>> 'WHEN' is a search condition, but I've used '1', not 'true'.
>> Also I tested it on PostgreSQL - they raise an error, so it is
>> both standard and practically used way.
>>
>> Below are my fixes for LIMIT and a small obvious refactoring,
>> but they are *not on the branch* - not all the tests pass when I
>> start banning non-bools in OP_If/IfNot.
> 
> I’ve fixed that.

But you still set OPFLAG_BOOLREQ and SQL_BOOLREQ. Why?
Also the commit message still describes this flag as a
key change of the patch.