[tarantool-patches] Re: [PATCH 3/6] sql: pass true types of columns to Tarantool

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

Hi! Thanks for the fixes! See 2 comments below.

>>> +		struct index_def *pk = pUseIndex;
>>>   		assert(pk != NULL);
>>>   -		uint32_t fieldno = pk->def->key_def->parts[0].fieldno;
>>> +		uint32_t fieldno = pk->key_def->parts[0].fieldno;
>>>   		enum affinity_type affinity =
>>>   			tab->def->fields[fieldno].affinity;
>>> -		if (pk->def->key_def->part_count == 1 &&
>>> +		if (pk->key_def->part_count == 1 &&
>>>   		    affinity == AFFINITY_INTEGER && (int)fieldno < nVector) {
>>>   			int reg_pk = rLhs + (int)fieldno;
>>
>> 3. Why do we still need this MustBeInt here? As I understand, here we
>> are trying to use an index for binary search of an LHS, but under the
>> hood it is just tarantool iterators starting from certain keys, it is
>> not? If it is, then tarantool iterators does key validation for us. Can
>> you please check if this opcode is redundant?
> 
> It doesn’t seem to be useless. Consider example:
> 
> CREATE TABLE t1 (id INT PRIMARY KEY);
> SELECT 1.23 IN t1;
> 
> Now, OP_Affinity doesn’t cast REAL to INT - I described reason
> in previous review. So, without OP_MustBeInt query above would
> result in
> 
> Execution failed: Supplied key type of part 0 does not match index part type: expected integer
> 
> Since 1.23 is passed to iterator over integer field. But I guess query above
> is fine - it should simply return 0, not an error since queries like
> 
> SELECT * FROM t1 WHERE id = 1.123;
> SELECT * FROM t1 WHERE id > 1.123;
> INSERT INTO t1 VALUES (1.123);
> 
> are allowed as well.

1. Then I am confused. Look at OP_MustBeInt. It calls
applyAffinity(pIn1, AFFINITY_NUMERIC). NUMERIC is real, so
1.23 cast to NUMERIC should be unchanged, it is not? I
tried this:

tarantool> box.sql.execute("select CAST(1.23 as NUMERIC)")
---
- - [1.23]
...

So 1.23 is already numeric. But applyAffinity function
for unknown reason does this: for target affinity ==
NUMERIC it sees that the original affinity is real and
casts the result to integer. So
applyAffinity(1.23, NUMERIC) == 1, but
SELECT CAST 1.23 as NUMERIC == 1.23.   ?!


> 
>>> diff --git a/test/sql-tap/boundary3.test.lua b/test/sql-tap/boundary3.test.lua
>>> index 5b63e0539..c5d605b65 100755
>>> --- a/test/sql-tap/boundary3.test.lua
>>> +++ b/test/sql-tap/boundary3.test.lua
>>> @@ -125,7 +125,7 @@ test:do_test(
>>>           -- </boundary3-1.3>
>>>       })
>>>   -test:do_execsql_test(
>>> +--[[test:do_execsql_test(
>>
>> 4. Do not silently comment tests. We should either delete it, or fix, or
>> open an issue to fix it later.
> 
> These tests are working. In previous review you asked to enable tests which
> are under ‘if false’, so I forgot about simply commented tests.

2. Sorry, in3.test.lua still has a commented test. orderby5.test.lua too.