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

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

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

> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index 3025e5e6b..107b9802d 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -1115,7 +1115,8 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,      /* Complete information about the W
>                 int seek_addr = 0;
>                 for (int i = 0; i < nEq; i++) {
>                         enum field_type type = idx_def->key_def->parts[i].type;
> -                       if (type == FIELD_TYPE_INTEGER) {
> +                       if (type == FIELD_TYPE_INTEGER ||
> +                           type == FIELD_TYPE_UNSIGNED) {
>                                 /*
>                                  * OP_MustBeInt consider NULLs as
>                                  * non-integer values, so firstly
> @@ -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.

>                         force_integer_reg = regBase + nEq;
>                 emit_apply_type(pParse, regBase, nConstraint - bSeekPastNull,
>                                 start_types);
> 
>>> 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".

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

>                                 sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i,
>                                               addrNxt);
>                         }
>                 }
>                 /* Inequality constraint comes always at the end of list. */
>