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

["n.pettik" <korablev@tarantool.org>]

>> @@ -1128,8 +1129,9 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,      /* Complete information about the W
>>                        }
>>                }
>>                /* Inequality constraint comes always at the end of list. */
>> -               if (pRangeStart != NULL &&
>> -                   idx_def->key_def->parts[nEq].type == FIELD_TYPE_INTEGER)
>> +               enum field_type ineq_type = idx_def->key_def->parts[nEq].type;
>> +               if (pRangeStart != NULL && (ineq_type == FIELD_TYPE_INTEGER ||
>> +                                           ineq_type == FIELD_TYPE_UNSIGNED))
> 
> 1. Please, add a test for unsigned. I thought that it is quite obvious -
> everything needs a test, especially if the bug was found during a review.

Surely, forgot about that:

diff --git a/test/sql/types.result b/test/sql/types.result
index bba39e320..383f93c1a 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -1012,3 +1012,35 @@ box.execute("SELECT a FROM t1 WHERE a IS NULL AND b IS NULL;")
 box.space.T1:drop()
 ---
 ...
+format = {}
+---
+...
+format[1] = { name = 'ID', type = 'unsigned' }
+---
+...
+format[2] = { name = 'A', type = 'unsigned' }
+---
+...
+s = box.schema.create_space('T1', { format = format })
+---
+...
+_ = s:create_index('pk')
+---
+...
+_ = s:create_index('sk', { parts = { 'A' } })
+---
+...
+s:insert({ 1, 1 })
+---
+- [1, 1]
+...
+box.execute("SELECT a FROM t1 WHERE a IN (1.1, 2.1);")
+---
+- metadata:
+  - name: A
+    type: unsigned
+  rows: []
+...
+s:drop()
+---
+...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index 16448436d..0218c5498 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -247,3 +247,14 @@ box.execute("SELECT a FROM t1 WHERE a = 1.5 AND b IS NULL;")
 box.execute("SELECT a FROM t1 WHERE a IS NULL AND b IS NULL;")
 
 box.space.T1:drop()
+
+format = {}
+format[1] = { name = 'ID', type = 'unsigned' }
+format[2] = { name = 'A', type = 'unsigned' }
+s = box.schema.create_space('T1', { format = format })
+_ = s:create_index('pk')
+_ = s:create_index('sk', { parts = { 'A' } })
+s:insert({ 1, 1 })
+box.execute("SELECT a FROM t1 WHERE a IN (1.1, 2.1);")
+
+s:drop()

>>>> 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.

>> Consequently, pRangeStart is NULL.
>> I’ve slightly updated this part of comment:
>> 
>> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
>> index 3025e5e6b..01ee9ce90 100644
>> --- a/src/box/sql/wherecode.c
>> +++ b/src/box/sql/wherecode.c
>> @@ -1089,10 +1089,10 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,    /* Complete information about the W
>>                 * 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.
>> +                * this step when it comes for less (<) comparison
>> +                * of nullable field. 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.
>> 
>> 
>> @@ -1112,24 +1112,33 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,    /* Complete information about the W
>>                                /*
>>                                 * OP_MustBeInt consider NULLs as
>>                                 * non-integer values, so firstly
>>                                 * check whether value is NULL or not.
>>                                 */
>> -                               seek_addr = sqlVdbeAddOp1(v, OP_IsNull,
>> +                               seek_addrs[i] = sqlVdbeAddOp1(v, OP_IsNull,
>>                                                          regBase);
> 
> 3. Now the indentation is broken.

diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index fde2efee0..4bd14bd5f 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1130,7 +1130,7 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,      /* Complete information about the W
                                 * check whether value is NULL or not.
                                 */
                                seek_addrs[i] = sqlVdbeAddOp1(v, OP_IsNull,
-                                                         regBase);
+                                                             regBase);
                                sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i,
                                              addrNxt);
                                start_types[i] = FIELD_TYPE_SCALAR;