[tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator

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

Thanks for the fixes!

>>>>> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
>>>>> index 977c0fced..e779729c7 100644
>>>>> --- a/src/box/sql/wherecode.c
>>>>> +++ b/src/box/sql/wherecode.c
>>>>> @@ -1086,31 +1086,61 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
>>>>> 			start_constraints = 1;
>>>>> 		}
>>>>> 		struct index_def *idx_pk = space->index[0]->def;
>>>>> -		int fieldno = idx_pk->key_def->parts[0].fieldno;
>>>>> -
>>>>> 		uint32_t pk_part_count = idx_pk->key_def->part_count;
>>>>> -		if (pk_part_count == 1 &&
>>>>> -		    space->def->fields[fieldno].type == FIELD_TYPE_INTEGER) {
>>>>> -			/* Right now INTEGER PRIMARY KEY is the only option to
>>>>> -			 * get Tarantool's INTEGER column type. Need special handling
>>>>> -			 * here: try to loosely convert FLOAT to INT. If RHS type
>>>>> -			 * is not INT or FLOAT - skip this ites, i.e. goto addrNxt.
>>>>> -			 */
>>>>> -			int limit = pRangeStart == NULL ? nEq : nEq + 1;
>>>>> -			for (int i = 0; i < limit; i++) {
>>>>> -				if (idx_def->key_def->parts[i].fieldno ==
>>>>> -				    idx_pk->key_def->parts[0].fieldno) {
>>>>> -					/* Here: we know for sure that table has INTEGER
>>>>> -					   PRIMARY KEY, single column, and Index we're
>>>>> -					   trying to use for scan contains this column. */
>>>>> -					if (i < nEq)
>>>>> -						sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i, addrNxt);
>>>>> -					else
>>>>> -						force_integer_reg = regBase + i;
>>>>> -					break;
>>>>> -				}
>>>>> +		/*
>>>>> +		 * Tarantool's iterator over integer fields doesn't
>>>>> +		 * tolerate floating point values. Hence, if term
>>>>> +		 * is equality comparison and value of operand is
>>>>> +		 * not integer, we can skip it since it always
>>>>> +		 * results in false: INT a == 0.5 -> false;
>>>>> +		 * It is done using OP_MustBeInt facilities.
>>>>> +		 * In case term is greater comparison (a > ?), we
>>>>> +		 * should notify OP_SeekGT to process truncation of
>>>>> +		 * floating point value: a > 0.5 -> a >= 1;
>>>>> +		 * It is done by setting P5 flag for OP_Seek*.
>>>>> +		 * It is worth mentioning that we do not need
>>>>> +		 * this step when it comes for less comparison:
>>>>> +		 * key is NULL in this case: values are ordered
>>>>> +		 * as  NULL, ... NULL, min_value, ..., so to
>>>>> +		 * fetch min value we pass NULL to GT iterator.
>>>>
>>>> 4. I did not get the 'less comparator == compare with NULL'
>>>> idea. Firstly you describe how to compare 'a > ?', this is
>>>> fully understandable. Then you, as I see, go to the 'a < ?'
>>>> case, but somewhy say, that it is the same as 'NULL < ?'.
>>>> Do not understand.
>>>
>>> I say that in this case key which is passed to iterator is NULL
>>> and type of iterator is GE.
>>
>> 2. Why? I still do not understand. What if I wrote 'a < 100'? you
>> do not replace 100 with NULL. But the comment says, that for any
>> 'less' operator a key is NULL: "when it comes for less comparison
>> key is NULL”.
> 
> Consider example:
> 
> box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);")
> box.execute("INSERT INTO t1 VALUES (1, 1), (2, 2), (3, -1), (4, NULL), (5, NULL);”)
> box.execute("SELECT a FROM t1 WHERE a < 100;”)
> 
> Bytecode for this query is:
> 
>  102>    0 Init             0    1    0               00 Start at 1
>  102>    1 IteratorOpen     1    1    0 space<name=T1> 00 index id = 1, space ptr = space<name=T1> or reg[0]; unique_unnamed_T1_2
>  102>    2 Explain          0    0    0 SEARCH TABLE T1 USING COVERING INDEX unique_unnamed_T1_2 (A<?) 00 
>  102>    3 Null             0    1    0               00 r[1]=NULL
>  102>    4 SeekGT           1   10    1 1             00 key=r[1]
>  102>    5 Integer        100    1    0               00 r[1]=100
>  102>    6 IdxGE            1   10    1 1             00 key=r[1]
>  102>    7 Column           1    1    2               00 r[2]=T1.A
>  102>    8 ResultRow        2    1    0               00 output=r[2]
>  102>    9 Next             1    6    0               00 
>  102>   10 Halt             0    0    0               00 
> 
> As you can see, SeekGT accepts NULL as a key for iterator.
> Then we iterate until search condition is true. So, basically
> 100 is replaced with NULL and not used as a key of iterator.
> I wanted to underline this fact in my comment.

Ah, thanks, now I got it.

Kirill, the patchset LGTM.