[tarantool-patches] Re: [PATCH 3/6] sql: pass true types of columns to Tarantool
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Oct 30 00:32:54 MSK 2018
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.
More information about the Tarantool-patches
mailing list